This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Adds more generic test macros.
ClosedPublic

Authored by Mordante on Jan 28 2023, 5:53 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG3476b56f0c78: [libc++][test] Adds more generic test macros.
Summary

These macros are intended to replace the macros in rapid-cxx-test.h.

Diff Detail

Event Timeline

Mordante created this revision.Jan 28 2023, 5:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2023, 5:53 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
Mordante updated this revision to Diff 492998.Jan 28 2023, 6:38 AM

Fixes CI,

ldionne added inline comments.
libcxx/test/support/assert_macros.h
56

I would use abort instead of exit -- some platforms can provide abort but can't provide exit, because exit has to be able to terminate the program successfully, which doesn't make sense on some platforms. abort() should be available on more platforms than exit.

59

It would also be nice to avoid mentioning std::cerr here. Would it make sense to pass a file descriptor like stderr to the functor being called so it can use std::fprintf(descriptor, ...) directly?

85–87

I would separate the exit out of the logging function. It would look like this ^.

90–94
110–113
125

Why not define this conditionally to nothing or to test_require? That would allow getting rid of test_libcpp_require above.

133
141

Please fully qualify throughout.

155

I feel that the filesystem order of arguments is clearer: exception type, exception predicate and finally the expression. Not a super strong opinion though.

163

To avoid using a non-reserved name?

Mordante marked 8 inline comments as done.Jan 31 2023, 12:37 PM

Thanks for the review!

libcxx/test/support/assert_macros.h
59

I don't think it makes sense to provide a descriptor. This should only log messages when an error occurs. Using printf(stderr, ...) instead of std::cerr, probably makes sense.

125

Good point!

163

I wouldn't mind using exception, but we shouldn't use reserved names in our tests. This would break the test when a standard library uses __exception.

Mordante updated this revision to Diff 493712.Jan 31 2023, 12:57 PM
Mordante marked an inline comment as done.

Addresses review comments.

Mordante updated this revision to Diff 494021.Feb 1 2023, 11:25 AM

CI fixes.

Mordante updated this revision to Diff 494326.Feb 2 2023, 8:54 AM

Rebased and a bit of polishing.

Mordante updated this revision to Diff 494895.Feb 5 2023, 3:11 AM

Rebased on D143203 and added documentation.

Mordante updated this revision to Diff 494897.Feb 5 2023, 4:22 AM

CI fixes.

Mordante updated this revision to Diff 495071.Feb 6 2023, 3:39 AM

Remove fs and unneeded format changes to keep this review to its basics.

Mordante retitled this revision from [libc++][test] Removes rapid-cxx-test.h. to [libc++][test] Adds more generic test macros..Feb 6 2023, 4:25 AM
Mordante edited the summary of this revision. (Show Details)
Mordante published this revision for review.Feb 6 2023, 5:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 5:03 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Feb 14 2023, 9:49 AM
libcxx/docs/TestingLibcxx.rst
153 ↗(On Diff #495071)
156–158 ↗(On Diff #495071)
159 ↗(On Diff #495071)

Etc.. It's less confusing for people if we consistently use paths from the root of the monorepo.

162–163 ↗(On Diff #495071)
164–169 ↗(On Diff #495071)
170–172 ↗(On Diff #495071)

I would avoid documenting that, since it's only useful for frequent contributors and they pretty much already know where to find those. I am a bit worried about some of this information becoming out of date.

228 ↗(On Diff #495071)
231 ↗(On Diff #495071)
279 ↗(On Diff #495071)
282 ↗(On Diff #495071)
303–309 ↗(On Diff #495071)

This looks like a footgun to me, I would not provide that. Hiding this sort of mechanism behind a macro seems like a bad idea to me.

I would instead return manually and add a comment. This isn't great, but the real solution would be to have a way to integrate this into lit so that lit can show us the test as being UNSUPPORTED based on a runtime condition, which is a much more involved thing to do.

libcxx/test/support/assert_macros.h
56

I don't think you actually need the string overload. That would allow you to drop the header dependency.

Mordante marked 11 inline comments as done.Feb 15 2023, 8:56 AM

Thanks for the review. I will look at the other comments after testing locally.

libcxx/docs/TestingLibcxx.rst
170–172 ↗(On Diff #495071)

I'm a bit on the fence. It's indeed most useful for frequent contributors, but not documented makes it harder for new contributors to become frequent contributors. Since I already have a link to libcxx/utils/libcxx/test/format.py it's quite easy to find this directory. So I'll remove it.

303–309 ↗(On Diff #495071)

As mentioned in our 1:1, this is added as compatibility with the existing rapid tests. As agreed I will remove this and update the test using this macro to use that approach.

Mordante marked 5 inline comments as done.Feb 15 2023, 9:38 AM
Mordante added inline comments.
libcxx/test/support/assert_macros.h
56

Good catch, you're correct. I also tested the follow-up patches and they keep working when removing the std::string overloads.

Mordante updated this revision to Diff 497710.Feb 15 2023, 9:39 AM
Mordante marked an inline comment as done.

Addresses review comments.

ldionne accepted this revision.Feb 16 2023, 1:11 PM

LGTM, but let's take a look at TEST_VALIDATE_EXCEPTION separately to see if we really want to keep it, and if so, under what form. This is good to commit, we can address it separately.

This revision is now accepted and ready to land.Feb 16 2023, 1:11 PM

LGTM, but let's take a look at TEST_VALIDATE_EXCEPTION separately to see if we really want to keep it, and if so, under what form. This is good to commit, we can address it separately.

Thanks for the review! Let's discuss it in our next 1:1.

This revision was automatically updated to reflect the committed changes.
libcxx/test/support/filesystem_test_helper.h