Opened 5 years ago

Last modified 5 years ago

#6437 defect new

twisted.trial.unittest.TestCase.flushWarnings may fail if run without .py files (with only .pyc or .pyo files)

Reported by: Jean-Paul Calderone Owned by: eddie
Priority: normal Milestone:
Component: trial Keywords: gsoc
Cc: Jonathan Lange Branch: branches/flushWarnings-with-pyc-6437
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description

flushWarnings includes logic to filter warnings by their source. This includes some filename comparison logic:

    # inspect.getabsfile(aFunction) sometimes returns a                                                                                        
    # filename which disagrees with the filename the warning                                                                                   
    # system generates.  This seems to be because a                                                                                            
    # function's code object doesn't deal with source files                                                                                    
    # being renamed.  inspect.getabsfile(module) seems                                                                                         
    # better (or at least agrees with the warning system                                                                                       
    # more often), and does some normalization for us which                                                                                    
    # is desirable.  inspect.getmodule() is attractive, but                                                                                    
    # somewhat broken in Python < 2.6.  See Python bug 4845.                                                                                   
    aModule = sys.modules[aFunction.__module__]
    filename = inspect.getabsfile(aModule)

    if filename != os.path.normcase(aWarning.filename):
        continue

inspect.getabsfile doesn't necessarily agree with the warning system either. If it discovers a ".pyc" filename and the corresponding ".py" file does not exist, it will return the ".pyc" file (if the ".py" file does exist, it strips the "c" off - which is why this isn't often a problem).

This is pretty easily reproduced. Write a test_foo.py file like this:

import warnings
from twisted.trial.unittest import TestCase

class X(TestCase):
    def test_y(self):
        warnings.warn("Stuff")
        self.assertEqual(1, len(self.flushWarnings([self.test_y])))

run it with trial to convince yourself it works (and to write a .pyc file). Then delete it, but leave the .pyc file. Run it again with trial and the test will fail instead.

Attachments (2)

6437.patch (2.9 KB) - added by eddie 5 years ago.
6437_2.patch (2.9 KB) - added by eddie 5 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

Changed 5 years ago by eddie

Attachment: 6437.patch added

comment:2 Changed 5 years ago by eddie

Keywords: review added

I think inspect.getabsfile is a infrastructure and does what it _ought_ to do, so it's not proper to modify it. Instead, I modified _synctest.py

Also, I add a unit test to make sure the modification is ok.

comment:3 Changed 5 years ago by Tom Prince

Author: tomprince
Branch: branches/flushWarnings-with-pyc-6437

(In [38121]) Branching to flushWarnings-with-pyc-6437.

comment:4 Changed 5 years ago by Tom Prince

(In [38122]) Apply 6437.patch from introom.

Refs: #6437

comment:5 Changed 5 years ago by Tom Prince

Keywords: gsoc added; review removed
Owner: set to eddie

I tried your patch against the buildbot, and there was a failure running with python 3.3. I'm not sure what is causing it, but I suspect that importlib.invalidate_caches is deleting the pyc file that we are trying to import.

Also

  1. The docstrings for test_missingSource and test_removedSource are quite similar (as are the tests). It would be useful to expand them somewhat, so it is clear what each is testing.
  2. This bugfix probably warrants a proper newsfile. Can you add one?

Note: I've applied your patch to a branch, so future diffs should be made against that branch.

Changed 5 years ago by eddie

Attachment: 6437_2.patch added

comment:6 in reply to:  5 Changed 5 years ago by eddie

Keywords: review added
Owner: eddie deleted

Replying to tom.prince:

I tried your patch against the buildbot, and there was a failure running with python 3.3. I'm not sure what is causing it, but I suspect that importlib.invalidate_caches is deleting the pyc file that we are trying to import.

I test it against trial and python3 admin/run-python3-test without failure. The reason why the previous patch fails under py3 is that in py3.3, pyc files are grouped into pycache, if we wanna load it, it should be put to where the source file resides and names it after the source file.

Also

  1. The docstrings for test_missingSource and test_removedSource are quite similar (as are the tests). It would be useful to expand them somewhat, so it is clear what each is testing.

test_missingSource is about import the module and then delete the source file. test_removedSource is about import the module under the situation that only the .pyc exists. I renamed test_removedSource to test_importFromPYC

  1. This bugfix probably warrants a proper newsfile. Can you add one?

I added an empty .misc (also the first patch contains an .misc, but isn't properly shown in this trac system)

Note: I've applied your patch to a branch, so future diffs should be made against that branch.

Thanks for your review.

comment:7 Changed 5 years ago by Tom Prince

  1. This bugfix probably warrants a proper newsfile. Can you add one?

I added an empty .misc

I meant a .bugfix.

comment:8 in reply to:  7 Changed 5 years ago by eddie

Replying to tom.prince:

  1. This bugfix probably warrants a proper newsfile. Can you add one?

I added an empty .misc

I meant a .bugfix.

Sorry, I should miss the word "BUGFIX". I will add one after it's been reviewed.

comment:9 Changed 5 years ago by Jean-Paul Calderone

(In [38440]) Apply 6437_2.patch

refs #6437

comment:10 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to eddie

Thanks.

  1. in twisted/trial/test/test_warnings.py
    1. Please use """ instead of ''' for docstrings
    2. There's a lot of duplication between test_missingSource, test_renamedSource, and test_importFromPYC. A start at fixing this might be to factor the code that creates the source for the helper package into a helper function that can be used from all three tests.
    3. Don't write FilePath(foo + b"/bar"). Instead, use FilePath(foo).child(b"bar")
    4. Also don't write FilePath(filePath.path). That's the same as filePath.
    5. The Python 3 support here needs some more polish. "cpython" is not the only supported runtime. Doesn't Python offer an API for constructing this path name? Alternatively, maybe we can get the pyc file some other way - some way that let's us put it exactly where we want it, instead of letting the runtime decide where it thinks it belongs. (Perhaps something in py_compile)
  2. in twisted/trial/test/test_warnings.py
    1. Thanks for considering the .pyo case as well. This should also have a unit test, though.

comment:11 in reply to:  10 Changed 5 years ago by eddie

Thanks for your review. If I understand the ticket right, then what we should address is the problem that flushWarnings will not work even though we have the pyc/pyo files (of course, the py file is deleted). Under cpython3, pycs are generated to pycache.If the pycache/foo.<magic>.pyc file exists, but the foo.py file used to create it does not, Python will raise an ImportError when asked to import foo. In other words, Python will not import a pyc file from the cache directory unless the source file exists. That is pretty obvious that to make things work, py source files are needed. (of course, their may be some ugly workarounds.) By using py_compile, we can generate pycs files to a specific place, and move it to the desired place to make flushWarnings is just a workaround to address this ticket. As a best practice, I think py source files should be remained, and the problems caused by omission of py source files is understandable.

comment:12 Changed 5 years ago by eddie

To conclude, py source files should be remained and addressing the problems made by the omission of py files under this ticket, I think, is not necessary.

comment:13 Changed 5 years ago by Jean-Paul Calderone

In other words, Python will not import a pyc file from the cache directory unless the source file exists.

Maybe that's true, but your previous patch added code to move a .pyc file around on Python 3 in order to get the test to pass.

Either .pyc-only operation is not important on Python 3, in which case the test shouldn't be doing special things to make itself pass on Python 3, or .pyc-only operation is important on Python 3 and the test needs to do something that doesn't tie it to specific CPython implementation details.

comment:14 in reply to:  13 Changed 5 years ago by eddie

Replying to exarkun:

In other words, Python will not import a pyc file from the cache directory unless the source file exists.

Maybe that's true, but your previous patch added code to move a .pyc file around on Python 3 in order to get the test to pass.

Yep, I shouldn't have done that for the only purpose of passing the test.

Either .pyc-only operation is not important on Python 3, in which case the test shouldn't be doing special things to make itself pass on Python 3,

Yes, I agree with that.

or .pyc-only operation is important on Python 3 and the test needs to do something that doesn't tie it to specific CPython implementation details.

comment:15 Changed 5 years ago by Jean-Paul Calderone

Okay, glad we agree on those points. :) So, looking a bit more at the behavior Python 3 implements, I see that import foo will succeed if there is a foo.pyc in a directory somewhere on sys.path. So ideally we would test this case as well. It will *not* succeed if there is only a __pycache__ entry for foo.

So I think that means we need to test the same configuration on Python 2 and on Python 3. We can ignore __pycache__ entirely, since you can't import a module it has bytecode in __pycache__ but no source file.

Note: See TracTickets for help on using tickets.