chrono
chrono copied to clipboard
chronotope
Add more tests for `parse_from_str` and specifiers
Add exhaustive set of tests for DateTime<FixedOffset>::parse_from_str function and all strftime specifiers.
Issue #1112
@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.
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!
@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.
I would like to see at least the tests from https://github.com/chronotope/chrono/pull/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 https://github.com/chronotope/chrono/pull/1083 and https://github.com/chronotope/chrono/pull/1094. Since it was your work, do you want to make a PR?
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.
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.
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?
@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.
I entirely forgot about this PR! 🙀 Let me get back to you, hopefully within a few weeks rather than months.
No worries :smile:.
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?