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 more tests for parse_from_str and specifiers #1122

Draft
wants to merge 1 commit into
base: 0.5.x
Choose a base branch
from

Conversation

jtmoon79
Copy link
Contributor

@jtmoon79 jtmoon79 commented Jun 3, 2023

Add exhaustive set of tests for DateTime<FixedOffset>::parse_from_str function and all strftime specifiers.

Issue #1112

@jtmoon79 jtmoon79 force-pushed the issue_1112_exhaustive_tests branch from fa15d84 to 26cd263 Compare June 3, 2023 21:48
@jtmoon79
Copy link
Contributor Author

jtmoon79 commented Jun 3, 2023

@djc this PR is not complete. Let me know if you'd consider accepting this and then I'll complete it (add tests for all strftime specifiers).

Before submitting a fix for PR #807 which caused Bug #1112, I'd like to create more exhaustive testing for parsing. There is already test test_strftime_items for parsing strftime specifiers. But test_strftime_items is focused on internal parsing APIs and did not catch #1112. I'd like to add exhaustive tests that focuses on the user-facing parse_from_str APIs. I may also add more asserts within test_strftime_items.

@pitdicker
Copy link
Collaborator

pitdicker commented Jun 4, 2023

I have to admit all the test failures when i was working on the parsing of %:z, %::z etc were highly annoying…. but also quite useful and I was thankful for them. But adding these tests is a large undertaking!

@pitdicker
Copy link
Collaborator

@jtmoon79 The last commit in https://github.com/pitdicker/chrono/commits/parse_padding is what I was thinking it would take to work with whitespace in combination with padding specifiers. Just putting it in here to spare some double work.

@pitdicker
Copy link
Collaborator

I would like to see at least the tests from #807 back on the 0.4.x branch. I don't think that part would be any problem, and it would go a long way in fixing the merge conflicts of #1083 and #1094. Since it was your work, do you want to make a PR?

@jtmoon79
Copy link
Contributor Author

jtmoon79 commented Jun 5, 2023

I would like to see at least the tests from #807 back on the 0.4.x branch. ... Since it was your work, do you want to make a PR?

I'll submit something in a few days or so.

@djc
Copy link
Member

djc commented Jun 5, 2023

I'd like to get away from the idea that more tests is always better -- lots of tests mean slower test runs and making the code harder to refactor. I think we should have coverage in place first, and then figure out how we improve coverage. IMO for a battery of new tests like this I also think it should be more obvious in the code what each test is supposed to cover.

@pitdicker
Copy link
Collaborator

pitdicker commented Feb 11, 2024

I don't feel like we have huge gaps in our coverage of parsing. More tests is not per se better, as we have to maintain and fix merge conflicts in tests like this (which currently takes more effort than I like and would have expected).

@jtmoon79 Do you want to create more targeted tests for parts of the code where we have low coverage? Either because of actual issues that point to such or the coverage data?

Add exhaustive set of tests for `DateTime<FixedOffset>::parse_from_str`
function and all strftime specifiers.

Issue chronotope#1112
@jtmoon79
Copy link
Contributor Author

@jtmoon79 Do you want to create more targeted tests for parts of the code where we have low coverage? Either because of actual issues that point to such or the coverage date?

I entirely forgot about this PR! 🙀
Let me get back to you, hopefully within a few weeks rather than months.

I don't feel like we have huge gaps in our coverage of parsing.

IMO #1112 is evidence this is not the case. I'd like to submit something for that. But the code has changed quite a lot so I need to fixup this PR.

@pitdicker
Copy link
Collaborator

I entirely forgot about this PR! 🙀
Let me get back to you, hopefully within a few weeks rather than months.

No worries 😄.

IMO #1112 is evidence this is not the case. I'd like to submit something for that.

Well that's a good argument.
I think for that issue the problem was that we don't have a test for our (lack of) support for padding specifiers. Just yesterday I was writing my notes on that into an issue #1425.

May I suggest a ca. 20-line test for the four cases of padding specifiers (default, none, zero or space) and their interaction with preceding whitespace or a trailing zero?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants