Skip to content

%b-%Y-%d #441

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.

Sign up for GitHub

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

Jump to bottom
Closed
jbrockmendel wants to merge 7 commits into from
Closed

%b-%Y-%d #441

jbrockmendel wants to merge 7 commits into from

Conversation

@jbrockmendel
Copy link
Contributor

@jbrockmendel jbrockmendel commented Aug 9, 2017

Apparently all the cool kids are transitioning to pytest. To get a feel for it I took a few of the more easily-parametrized cases from test_parser.py and implemented them in test_parser_parametrized.py. Explicitly looping over all the valid formats exposed a handful that are not currently handled correctly (and clearly, not currently tested). Namely:

from dateutil import parser
>>> parser.parse('Sept 2003 25')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/site-packages/dateutil/parser.py", line 1182, in parse
    return DEFAULTPARSER.parse(timestr, **kwargs)
  File "/usr/local/lib/python2.7/site-packages/dateutil/parser.py", line 581, in parse
    ret = default.replace(**repl)
ValueError: day is out of range for month

This happens because resolve_ymd never considers the possibility (month, year, day). This PR adds that particular check.

@jbrockmendel
Use pytest for parameterized tests, implement basic ISO test cases as…
2abc9e6 
… parametrized tests

Adjust tox.ini and .gitignore accordingly
@jbrockmendel
Remove form test_parser.py tests that are now handled by test_parser_…
1587de0 
…parametrized

Fix resolve_ymd for case that fails under new tests
@jbrockmendel
Simpler fix for resolve_ymd; add coveragerc
0e02a21
@pganssle
Copy link
Member

pganssle commented Aug 9, 2017

@jbrockmendel I would say that there's no reason to break test_parser into two files. I've been meaning to switch over to parameterized tests for a while now - I've been doing something like that in a half-assed way until I got around to actually adding the dependency.

Anyway, the CI is broken because I think you need to add a tests_require line into the setup.py.

pganssle
pganssle reviewed Aug 10, 2017
View reviewed changes
dateutil/test/test_parser.py Outdated
token_opts = [['%Y'], ['%b', '%B'], ['%d', '%-d']]

prods = itertools.product(*token_opts)
perms = [y for x in prods for y in itertools.permutations(x)]
Copy link
Member

@pganssle pganssle Aug 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if this is more readable per se, but I believe this is equivalent to:

list(itertools.chain.from_iterable(map(itertools.permutations, itertools.prod(*token_opts))))

or possibly better:

list(itertools.chain(*(itertools.permutations(x) for x in itertools.prod(*token_opts))))

Copy link
Contributor Author

@jbrockmendel jbrockmendel Aug 10, 2017

Choose a reason for hiding this comment

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

I find the two-line version more readable, but am pretty close to indifferent on this.

@jbrockmendel
Copy link
Contributor Author

jbrockmendel commented Aug 10, 2017

pytest doesn't allow for yield-based tests, right? I tried having this test run for (much) more than just 2003-09-25 and it looks like it created the tests as a list rather than a generator.

@pganssle
Copy link
Member

pganssle commented Aug 14, 2017 •
edited
Loading

I can't reproduce this test failure on my local machine with Python 3.4 on Linux. Is this a travis thing?

@jbrockmendel
Copy link
Contributor Author

jbrockmendel commented Aug 16, 2017

The Travis errors look like they are coming from sys.modules.values() changing size in iteration. Is this showing up elsewhere? If not, I'd guess that pytest isn't playing nicely with... something in py 3.2, 3.3, 3.4.

@pganssle
Copy link
Member

pganssle commented Aug 16, 2017

Yeah, that was what I figured, but I'm not sure what because my machine doesn't do it when I spin up a Python 3.4 conda environment. I was thinking I'd maybe try removing the profiling stuff, then the coverage stuff sequentially and see if either of those fixes it, at least to narrow it down.

@jbrockmendel
Copy link
Contributor Author

jbrockmendel commented Aug 26, 2017

I just pushed a commit that uses regular nose-style tests instead of pytest. We'll see if that was the issue with the Travis error.

@pganssle
Copy link
Member

pganssle commented Aug 26, 2017

@jbrockmendel What was it you changed that fixed this? I just found out about debug mode on TravisCI, so I was thinking I'd try and debug it that way.

@jbrockmendel
Copy link
Contributor Author

jbrockmendel commented Aug 26, 2017

I just removed pytest and changed the new test to yield nose-style.

@pganssle
Copy link
Member

pganssle commented Sep 1, 2017

@jbrockmendel I'll try and look at this in more detail this weekend, I'm not sure I understand how these yield tests work, and it seems like you are still actually using pytest here. You didn't squash the old broken commit, right? I'm going to try and figure out why it was broken in the first place.

@jbrockmendel
Copy link
Contributor Author

jbrockmendel commented Sep 3, 2017

I think you're right that I didn't completely remove pytest. I can make a new PR that just fixes the bug and adds the tests, completely stripping out the other changes. Let me know your preference.

@jbrockmendel
Copy link
Contributor Author

jbrockmendel commented Oct 25, 2017

I'd like to wrap up at least the bug-fix part of this. Is the pytest problem still a problem? If so, I'll make a new PR that uses the older test setup.

@pganssle
Copy link
Member

pganssle commented Oct 25, 2017

This is the most infuriating thing. I managed to open an interactive shell, but I still can't replicate the problem.

Anyway, yeah, I think just drop both all the pytest stuff including the yield tests. Switch that from using assert to using self.assertEqual. Normally I don't like a single test that handles a bunch of test cases, but as a temporary matter before we switch over to pytest and properly parametrized tests, we can just generate the tests inside the test function.

@jbrockmendel jbrockmendel mentioned this pull request Oct 26, 2017
Merged
@pganssle
Copy link
Member

pganssle commented Oct 27, 2017

Superceded by #481

@pganssle pganssle closed this Oct 27, 2017
@pganssle pganssle added this to the wontfix milestone Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pganssle pganssle pganssle left review comments

Assignees

No one assigned

Labels

bug parser tests

Projects

None yet

Milestone

wontfix

Development

Successfully merging this pull request may close these issues.

2 participants

@jbrockmendel @pganssle