Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for boundary conditions when computing ISO week number #90

Merged
merged 1 commit into from
Apr 2, 2023

Conversation

lopopolo
Copy link
Member

@lopopolo lopopolo commented Apr 2, 2023

These tests were pulled from the following PR in chrono:

Add a single test with these boundary condition dates. The dates were validated with MRI Ruby v3.1.2. The third test case differs between chrono and MRI, but since we target MRI, use it as the source of truth.

No logic was changed as part of this commit, only additional tests were added.

@lopopolo lopopolo added A-formatter Area: strftime formatting. C-bug Category: This is a bug. labels Apr 2, 2023
@lopopolo
Copy link
Member Author

lopopolo commented Apr 2, 2023

Hi @x-hgg-x, I tried to dig into the week_number and iso_week_number functions to see what's going wrong but I don't quite understand all of the tricky math. Would you be able to take a look?

src/tests/format.rs Outdated Show resolved Hide resolved
src/tests/format.rs Outdated Show resolved Hide resolved
@x-hgg-x
Copy link
Collaborator

x-hgg-x commented Apr 2, 2023

Since you are testing multiple formatting specifiers at the same time, you must ensure that all fields of the MockTime are consistent and represent a valid date and time. We are currently doing light checks in the CheckedTime trait, but it is only to prevent panics due to overflow, and we don't validate the consistency of all fields.

@lopopolo
Copy link
Member Author

lopopolo commented Apr 2, 2023

It looks like that did the trick! Thanks for the help 🙇

@lopopolo lopopolo requested a review from x-hgg-x April 2, 2023 16:30
@x-hgg-x
Copy link
Collaborator

x-hgg-x commented Apr 2, 2023

Please remember to squash the commits and modify their description, since we didn't actually change any code.

These tests were pulled from the following PR in chrono:

- chronotope/chrono#966

Add a single test with these boundary condition dates. The dates were
validated with MRI Ruby v3.1.2. The third test case differs between
chrono and MRI, but since we target MRI, use it as the source of truth.

No logic was changed as part of this commit, only additional tests were
added.

Co-authored-by: x-hgg-x <39058530+x-hgg-x@users.noreply.github.com>
@lopopolo lopopolo force-pushed the lopopolo/iso-week-regression-tests branch from 7c9eed7 to 6d002a6 Compare April 2, 2023 17:12
@lopopolo lopopolo added E-needs-test Call for participation: Writing correctness tests. and removed C-bug Category: This is a bug. labels Apr 2, 2023
@lopopolo lopopolo merged commit 8c6301a into trunk Apr 2, 2023
@lopopolo lopopolo deleted the lopopolo/iso-week-regression-tests branch April 2, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-formatter Area: strftime formatting. E-needs-test Call for participation: Writing correctness tests.
Development

Successfully merging this pull request may close these issues.

2 participants