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

4.0a3 introduces an "Unexpected third case" error in the Nose test suite... #353

Closed
nedbat opened this issue Jan 25, 2015 · 18 comments
Closed
Labels
bug Something isn't working

Comments

@nedbat
Copy link
Owner

nedbat commented Jan 25, 2015

Originally reported by jszakmeister (Bitbucket: jszakmeister, GitHub: jszakmeister)


While running the test suite for Nose, I've run into an "Unexpected third case" error. I'm not sure why this is occurring. The tests tripping up coverage are located here: https://github.com/nose-devs/nose/tree/master/functional_tests/support/coverage2. Also, this occurs under Python 3.4, but not under 2.7.

Let me know if there's something more I can do.


@nedbat
Copy link
Owner Author

nedbat commented Jan 25, 2015

@jszakmeister Can you provide more details? It would be helpful to have the output of the test run, how I can run the tests myself, what platform you are on, etc.

@nedbat
Copy link
Owner Author

nedbat commented Jan 25, 2015

Original comment by jszakmeister (Bitbucket: jszakmeister, GitHub: jszakmeister)


Sorry, I didn't have much at the time as I was troubleshooting a different issue.

I've managed to put together a minimal test case that mimics what Nose is doing in its test suite. I've put the necessary files in a Gist: https://gist.github.com/jszakmeister/0960bf7b6434de40328f

Under Python 3.4, I see:

#!text

:: python break-cov.py
Traceback (most recent call last):
  File "break-cov.py", line 19, in <module>
    inst.save()
  File "/Users/jszakmeister/.virtualenvs/cov-test/lib/python3.4/site-packages/coverage/control.py", line 663, in save
    self._harvest_data()
  File "/Users/jszakmeister/.virtualenvs/cov-test/lib/python3.4/site-packages/coverage/control.py", line 716, in _harvest_data
    pkg, sys.modules[pkg], sys.modules[pkg].__file__
AssertionError: Unexpected third case: name = foo, object = <module 'foo' from '/Users/jszakmeister/tmp/cov-break/foo.py'>, __file__ = /Users/jszakmeister/tmp/cov-break/foo.py

I'm running under Mac OS X (Yosemite 10.10.1), but I imagine the problem exists on other platforms too.

@nedbat
Copy link
Owner Author

nedbat commented Jan 25, 2015

Original comment by jszakmeister (Bitbucket: jszakmeister, GitHub: jszakmeister)


I should add it actually first shows up with coverage 4.0a2. The issue is not present with 4.0a1.

@nedbat
Copy link
Owner Author

nedbat commented Jan 25, 2015

I see what you mean, thanks for the simple test case. Part of the reason this assert happens is because the second "import foo" doesn't actually execute foo.py, since it's already been imported once. Do you need to execute the foo.py code? Even if we fix that "third-case" assert, the second coverage results won't include foo.py. Is that what you expect?

@nedbat
Copy link
Owner Author

nedbat commented Jan 25, 2015

I could easily make coverage output this: "Coverage.py warning: foo seems to have been previously imported, but unmeasured." Is that enough of a fix?

@nedbat
Copy link
Owner Author

nedbat commented Jan 25, 2015

Original comment by jszakmeister (Bitbucket: jszakmeister, GitHub: jszakmeister)


It could be that I just need to do something different in for our test cases too. My fear was that something broke in an unexpected way, that's why I wanted to let you know. I could probably work around this a few different ways on my end, so I think it's probably more about what you think is right. For me, a warning would probably be kinder, but if it isn't going to work as expected, I'll probably resort to launching it externally.

@nedbat
Copy link
Owner Author

nedbat commented Jan 25, 2015

I looked at how the code has changed. In the old code, during the second coverage run in break-cov.py, it would see that foo was in sys.modules, and decide that it had been covered, so no warning was needed. But in fact, no lines from foo.py had been collected. They are in the coverage data because your two runs are loading and saving the data, so they aggregate all the runs.

I don't know what your real code is like, so I don't know if its right to silently accept foo has having been covered, even though it wasn't run, or to warn that a module you said you cared about wasn't actually measured.

@nedbat
Copy link
Owner Author

nedbat commented Jan 25, 2015

Original comment by jszakmeister (Bitbucket: jszakmeister, GitHub: jszakmeister)


It's just tests mainly covering the reporting features, so the merging results would be fine as would not actually re-importing anything. Nose (long before I took it over) runs many of its tests in the same process, but they mainly cover reporting aspects--there should be no differences in the actual lines covered. In the real world, things wouldn't go that way since each test run would start a new process, and the imports would actually happen. I could probably forcefully evict them too, but I don't think it's necessary for what we are doing.

@nedbat
Copy link
Owner Author

nedbat commented Jan 26, 2015

I'm not sure what you need me to do. I think I have to change the code somehow, there shouldn't be an assert there, since there's clearly a way to make it happen.

@nedbat
Copy link
Owner Author

nedbat commented Jan 26, 2015

Original comment by jszakmeister (Bitbucket: jszakmeister, GitHub: jszakmeister)


Sorry, I've tried not to dictate a solution since it's your project and you
have a better grasp about what Coverage should and shouldn't do.

I agree, I think the assert needs to go too, but I do think logging the
issue is a good idea though. Both of which are good enough for me. Thanks
Ned!

@nedbat
Copy link
Owner Author

nedbat commented Jan 26, 2015

Original comment by Buck Evan (Bitbucket: bukzor, GitHub: bukzor)


+1 to change the assert to a warning.
Can we make it yellow in the case of isatty(stdout), ala pip?

@jszakmeister Could you give us your best guess at ideal, expected behavior in this case?
If we continue down this route, the second coverage would result in a warning and no measurements.

@nedbat
Copy link
Owner Author

nedbat commented Jan 26, 2015

@nedbat
Copy link
Owner Author

nedbat commented Jan 26, 2015

Original comment by Buck Evan (Bitbucket: bukzor, GitHub: bukzor)


It occurs to me that zak will encounter a lot of warning spam that he didn't have before.
I've encountered a similar situation in my project, and had to jump through some brittle hoops to deal with it:
https://github.com/Yelp/venv-update/blob/master/tests/testing/\_\_init\_\_.py#L65

@nedbat
Copy link
Owner Author

nedbat commented Jan 26, 2015

Original comment by jszakmeister (Bitbucket: jszakmeister, GitHub: jszakmeister)


@bukzor As I said before, my expectations are pretty small. If nothing is imported after the fact, then I think it's perfectly acceptable to say "no measurements were taken." My only real concern was that I hit an edge that was causing an assert and it wasn't clear to me how I was violating an API and it felt like it was an edge that Ned didn't think was possible (since he had an assert present).

All of that said, fixing this has no bearing on the normal operation of Nose--Nose only creates one instance during the course of normal operation. I'd prefer Ned to do whatever is best for the project and its goals.

@nedbat
Copy link
Owner Author

nedbat commented Jan 27, 2015

@bukzor Thanks for the pull request, though I'd prefer to also have a test or two.

About the warnings: when I first put them in, I wondered if there should be a way to quiet them.

@nedbat
Copy link
Owner Author

nedbat commented Jan 27, 2015

Fixed in 283e6b95c5bc (bb)

@nedbat
Copy link
Owner Author

nedbat commented Jan 27, 2015

Original comment by Buck Evan (Bitbucket: bukzor, GitHub: bukzor)


@nedbat The link is bad somehow, and I don't find it in the commit list.

@nedbat
Copy link
Owner Author

nedbat commented Jan 27, 2015

oops: forgot to push. It's there now.

@nedbat nedbat closed this as completed Jan 27, 2015
@nedbat nedbat added major bug Something isn't working labels Jun 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant