This is an archive of the discontinued LLVM Phabricator instance.

[NFC][libc++][format] Rename tests.
AbandonedPublic

Authored by Mordante on Dec 7 2022, 11:05 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

The number of handle based formatters in the Standard increases with new
papers being added. Ideally these papers have their own tests. This
renames the existing tests and adds a common header with elements used
in all test.

Diff Detail

Event Timeline

Mordante created this revision.Dec 7 2022, 11:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 11:05 AM
Mordante requested review of this revision.Dec 7 2022, 11:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 11:05 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 481284.Dec 8 2022, 8:13 AM

Retrigger CI.

ldionne requested changes to this revision.Dec 8 2022, 12:14 PM
ldionne added a subscriber: ldionne.

This is unusual for our test suite. We normally test by function, not by paper, and I think there is value in keeping consistency here. Is there a reason why this fundamentally can't be done (or is much more complicated to do) with the current organization?

This revision now requires changes to proceed.Dec 8 2022, 12:14 PM

This is unusual for our test suite. We normally test by function, not by paper, and I think there is value in keeping consistency here. Is there a reason why this fundamentally can't be done (or is much more complicated to do) with the current organization?

I know this is unusual. I hoped the explanation in the commit message was enough to clarify it.
When the Standard adds new formatters, these have a parse and format member. These can be tested separately, but there should be a test for the interaction. The easiest way to do so is using std::format and std::vformat. Both are needed since std::format tests the constexpr validation and std::vformat the runtime validation.

The current header with tests is about 3000 lines and the test takes quite a bit of time to execute. Adding a lot more tests for the range-based formatters makes running these tests slower, which is quite annoying during development. The current tests have quite a bit of duplicated testing while not adding a lot of extra test coverage. I can't remove them since all format functions should be tested. For the range-based tests I don't feel they need to do all these extra test. (The extra tests are format_to, formatted_to_n, formatted_size and their 'v' partners.)

Next to the C++23 range-based formatter I expect several new library formatters in C++26.

If you disagree of have other suggestions, maybe discuss it during our next meeting.

Per our discussion just now, I would suggest testing e.g. container formatters in the section of the standard where they are defined, not in the tests for format itself. You could split out some of the commonality into libcxx/test/support.

Mordante abandoned this revision.Dec 13 2022, 10:21 AM

Per our discussion just now, I would suggest testing e.g. container formatters in the section of the standard where they are defined, not in the tests for format itself. You could split out some of the commonality into libcxx/test/support.

I'll look at that in a separate review, abandoning this one for now.