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

Issue #353 resolved
John Szakmeister created an issue

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.

Comments (18)

  1. Ned Batchelder repo owner

    @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.

  2. John Szakmeister reporter

    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:

    :: python break-cov.py
    Traceback (most recent call last):
      File "break-cov.py", line 19, in <module>
      File "/Users/jszakmeister/.virtualenvs/cov-test/lib/python3.4/site-packages/coverage/control.py", line 663, in save
      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.

  3. John Szakmeister reporter

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

  4. Ned Batchelder repo owner

    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?

  5. Ned Batchelder repo owner

    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?

  6. John Szakmeister reporter

    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.

  7. Ned Batchelder repo owner

    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.

  8. John Szakmeister reporter

    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.

  9. Ned Batchelder repo owner

    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.

  10. John Szakmeister reporter

    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!

  11. Buck Evan

    +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.

  12. John Szakmeister reporter

    @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.

  13. Ned Batchelder repo owner

    @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.

  14. Log in to comment