Skip to content

P0355R7_calendars_and_time_zones_io: Cleanup test coverage#6280

Open
StephanTLavavej wants to merge 11 commits intomicrosoft:mainfrom
StephanTLavavej:chronos-most-wanted
Open

P0355R7_calendars_and_time_zones_io: Cleanup test coverage#6280
StephanTLavavej wants to merge 11 commits intomicrosoft:mainfrom
StephanTLavavej:chronos-most-wanted

Conversation

@StephanTLavavej
Copy link
Copy Markdown
Member

@StephanTLavavej StephanTLavavej commented May 8, 2026

Followup to in-flight #6270; the first commit here will merge away harmlessly.

This is in response to a good Copilot code review suggestion; all of the edits (and regexes) are mine.

  • Add want_value().
    • This makes it harder to forget to inspect the parsed value, and makes the structure of repetitive tests clearer.
    • Most, but not all, calls to test_parse() can be converted. The exceptions are when we need to parse abbrev/offset, or the assert has to do something unusual. As always, making the common cases simple helps the unusual cases stand out more.
  • assert\((.+) && (.+)\); => assert($1); assert($2);
    • We try to avoid multi-asserts because they make the causes of failures harder to see.
  • test_parse\((.+), (\w+)\);\n *assert\(\2 == (.+)\); => want_value($1, $2, $3);
    • Mechanically convert callsites.
  • test_parse\((.+), (\w+)\); // (.+)\n *assert\(\2 == (.+)\); => want_value($1, $2, $4); // $3
    • Mechanically handle the callsites with comments on the test_parse() line. (The earlier commit correctly handled comments on the assert() line, and no callsites had comments on both lines.)
  • Manually handle asserts that needed extra parens.
  • Manually handle asserts that called .count(); we can instead construct values of the expected types.
  • Manually add wanted values that weren't being inspected.
  • Manually handle asserts that were batched up.
  • Manually handle a confusing assert.
    • After parsing gt and verifying that it was equal to clock_cast<gps_clock>(ref), this parsed tt and then inspected it via clock_cast<gps_clock>, wanting it to be equal to gt. That's really confusing. Instead of performing the comparison as gps_clock, and involving the previously-parsed gt, let's go back to the original ref and view it through clock_cast<tai_clock>.
  • Use source_location in want_value().
    • Copilot code review is working! 😹 Example after damaging the test:
      want_value() encountered an undesired value on line 261.
      Assertion failed: got_wanted, file D:\GitHub\STL\tests\std\tests\P0355R7_calendars_and_time_zones_io\test.cpp, line 203
      

After parsing `gt` and verifying that it was equal to `clock_cast<gps_clock>(ref)`,
this parsed `tt` and then inspected it via `clock_cast<gps_clock>`, wanting it to be equal to `gt`.
That's really confusing. Instead of performing the comparison as `gps_clock`,
and involving the previously-parsed `gt`, let's go back to the original `ref`
and view it through `clock_cast<tai_clock>`.
Copilot AI review requested due to automatic review settings May 8, 2026 20:58
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner May 8, 2026 20:58
@StephanTLavavej StephanTLavavej added test Related to test code chrono C++20 chrono labels May 8, 2026
@github-project-automation github-project-automation Bot moved this to Initial Review in STL Code Reviews May 8, 2026
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in STL Code Reviews May 8, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors and extends the <chrono> parsing IO tests for P0355R7_calendars_and_time_zones_io, primarily to make repetitive parse-and-compare assertions clearer and to add targeted coverage for modified-width parsing edge cases (follow-up to the in-flight work in #6270).

Changes:

  • Introduces want_value() and converts many test_parse() + assert(value == expected) sequences to a single helper call; also splits combined asserts for clearer failure causes.
  • Improves test diagnostics for unexpected parse success/failure by threading std::source_location into test_parse() / fail_parse().
  • Adds parse_modified_maximum_characters() coverage and includes small bounds-check adjustments in the underlying parsing helpers (stl/inc/chrono, stl/inc/xloctime) to preserve room for null terminators.
Show a summary per file
File Description
tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp Refactors parse tests via want_value(), improves diagnostics with source_location, and adds modified-width parsing coverage.
stl/inc/xloctime Adjusts digit-copy logic to preserve space for a null terminator (bounds safety).
stl/inc/chrono Adjusts _Get_int() digit-copy bounds to preserve space for a null terminator (bounds safety).

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 1

Comment thread tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chrono C++20 chrono test Related to test code

Projects

Status: Final Review

Development

Successfully merging this pull request may close these issues.

2 participants