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+collect tests for tzinfos input types, fix missed case of invalid… #891

Merged
merged 6 commits into from Mar 20, 2019

Conversation

jbrockmendel
Copy link
Contributor

Implement tests for types of inputs that can be passed as tzinfos (two of these are moved from further down in the file, the rest ported from downstream). Fixes the case where something bizarre is passed like tzinfos={"UTC": something_totally_invalid} which at the moment raises UnboundLocalError instead of TypeError

@pganssle pganssle changed the title TST: add+collect tests for tzinfos input types, fix missed case of invalid… Add+collect tests for tzinfos input types, fix missed case of invalid… Feb 27, 2019
Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

Minor changes here, and this also needs a changelog (in the end I'm probably going to do a rollup of this in the next release to merge them into one big "Added a ton of tests. Patches by @jbrockmendel (gh prs #x, #y, #z)" so they can be pretty terse.

dateutil/test/test_parser.py Outdated Show resolved Hide resolved
dateutil/test/test_parser.py Outdated Show resolved Hide resolved
dateutil/test/test_parser.py Outdated Show resolved Hide resolved
@jbrockmendel
Copy link
Contributor Author

I'm travelling the next couple days, so may not get around to this until Wednesday. In the interim, can we merge the other two?

@pganssle
Copy link
Member

pganssle commented Mar 4, 2019

@jbrockmendel I merged #892, but I think #894 is still blocked on changes from you. If you want I can make the changes myself and merge that one.

@jbrockmendel
Copy link
Contributor Author

I think the requested change here is just adding assert res.tzinfo is expected.tzinfo where appropriate. I'm skipping that for the case where both are None. That leaves test_valid_tzinfo_tzinfo_input, test_valid_tzinfo_callable_input where that passes and test_valid_tzinfo_unicode_input where that assertion fails since that assertion amounts to tz.tzstr('UTC+0') is tz.UTC. I'm leaving that assertion out in the newest commit.

@jbrockmendel
Copy link
Contributor Author

@pganssle gentle ping. I think this is the last of my test-refactoring PRs.

Also definitely-ready is gfyoung's #881. I definitely don't want to merge any of my own PRs, but would be comfortable taking on merge responsibilities for things like that.

@pganssle
Copy link
Member

@jbrockmendel Sorry about all the delays. I'll review again today or tomorrow. Assuming any changes I might want are sufficiently simple, I'll just push them directly to your branch and merge rather than bother you with another round of addressing review comments.

@pganssle
Copy link
Member

pganssle commented Mar 19, 2019

@jbrockmendel I've rebased this and unified the "check if equal + check if tzinfo is identical" logic into a common function, but looking at it, I think I misunderstood the xfail test:

    def test_valid_tzinfo_int_input(self):
        dstr = "2014 January 19 09:00 UTC"
        tzinfos = {u"UTC": -28800}
        expected = datetime(2014, 1, 19, 9, tzinfo=tz.UTC)
        res = parse(dstr, tzinfos=tzinfos)
        self.assert_equal_same_tz(res, expected)

The current behavior is that tz.tzoffset("UTC", -28800) gets attached to the result, which seems like the right thing to do, since specifying UTC in tzinfos is more explicit than an automatic mapping of "UTC" to tz.UTC.

I'll note that the fact that tzinfos can override what "UTC" gets parsed to also enables you to do things like tzinfos={"UTC": datetime.timezone.utc}, which could be very useful for people who want to use the standard library's objects.

If you agree that we should keep the current behavior, I'll adjust the final test appropriately and merge.

@jbrockmendel
Copy link
Contributor Author

Yes, I agree that if the user explicitly specifies a non-standard UTC, that should take precedence.

jbrockmendel and others added 6 commits March 20, 2019 09:22
The `tzinfos` argument allows you to add or override the mappings
between time zone strings and tzinfo objects, this PR groups the
existing tests for this argument together in a class, and expands on it
to a few more cases.
Currently, passing an argument to `tzinfos` that cannot be interpreted
as a time zone will raise UnboundLocalError, but it should raise
TypeError.
This switches the error from UnboundLocalError to TypeError.
For all of these tests, the most important part of the test is ensuring
that the correct tzinfo object was attached, so it makes sense to add a
convenience function that asserts that they are identical *and* have the
same tzinfo object.
Co-authored-by: Brock Mendel <jbrockmendel@gmail.com>
Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

OK, I've tweaked and rewritten the commit history of this PR to my nitpicky satisfaction, ready for merge when CI passes.

@pganssle pganssle merged commit 9e95111 into dateutil:master Mar 20, 2019
@pganssle
Copy link
Member

Thanks @jbrockmendel for the patience. Now to continue working through the backlog on all my projects...

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.

None yet

2 participants