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

CR fails to kill a mutant when the mutated module is loaded via importlib #157

Closed
atodorov opened this issue Jul 14, 2016 · 18 comments

Comments

@atodorov
Copy link
Contributor

commented Jul 14, 2016

Please see this for reproducer:
atodorov/mutation-testing-example@8f8a3ab

I have a sandwich/ham/ham.py module. The Ham class is loaded in sandwich/control.py using importlib.import_module. Note that the loaded module is in a subdirectory and we modify sys.path - this seems to be related to the problem.

Then I have test_control.py which grabs the ham_class from control.py and creates an object of this type, then proceeds to assert on the default values of this object.

The execute the test suite manually run python3 -m unittest tests/test_control.py - everything is fine.
If I make the below mutation by manually editing the code and re-run the test suite it fails as expected:

$ python3 -m unittest tests/test_control.py 
F
======================================================================
FAIL: test_loading_via_importlib (tests.test_control.TestControl)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/atodorov/mutation-testing-example/tests/test_control.py", line 7, in test_loading_via_importlib
    self.assertEqual(ham_in_fridge.pieces, 10)
AssertionError: 11 != 10

----------------------------------------------------------------------
Ran 1 test in 0.000s

FAILED (failures=1)

However when running CR the very same mutation leads to a surviving mutant:

$ cosmic-ray run --baseline=10 ham.json sandwich/ham/ham.py -- tests

$ cosmic-ray report ham.json
job ID 1:Outcome.SURVIVED:sandwich.ham.ham
command: cosmic-ray worker sandwich.ham.ham number_replacer 0 unittest -- tests
--- mutation diff ---
--- a/home/atodorov/mutation-testing-example/sandwich/ham/ham.py
+++ b/home/atodorov/mutation-testing-example/sandwich/ham/ham.py
@@ -3,6 +3,6 @@

 class Ham(object):

-    def __init__(self, pieces=10):
+    def __init__(self, pieces=11):
         self.pieces = pieces

total jobs: 1
complete: 1 (100.00%)
survival rate: 100.00%
@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2016

Ah, this is interesting. This could be complex-to-impossible, and maybe it'll require monkey patching to be completely watertight. I'm not sure when I'll have time to really look into it, but hopefully soon.

@abingham abingham added the bug label Jul 14, 2016

@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2016

I doesn't appear that importlib.import_module() somehow bypasses the sys.meta_path finders. This code indicates that the finder is being invoked by import_module():

from importlib.abc import MetaPathFinder
import sys


class MyFinder(MetaPathFinder):
    def find_spec(self, fullname, path, target=None):
        print('MyFinder.find_spec():', fullname, path, target)
        return None


finder = MyFinder()
sys.meta_path = [finder] + sys.meta_path

import importlib
importlib.import_module('ham')

So there must be something a bit deeper at play...

@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2016

I think I see what's going on. In your control.py you manipulate sys.path to put sandwich/ham at the front, thereby allowing you to import the module ham. In other words, you're asking Python to import the module ham, and you're jumping through a few hoops to make this possible.

On the other hand, you're asking CR to mutate the package sandwich, including its submodule sandwich.ham.ham. So from CR's point of view, there is no module called ham, just sandwich.ham.ham.

As a result, CR is creating a mutant of the code in sandwich/ham/ham.py and inserting it as the module sandwich.ham.ham. Meanwhile, you're using importlib to import the module ham and CR has not been asked to do anything with a module by that name.

This is just based on reading the code; I can't run CR where I am right now. However, I think you could verify this by changing control.py. Remove the manipulations of sys.path and, instead of importing "ham", import "sandwich.ham.ham". I think that you'll find then that the mutation takes effect in your tests. The code would look something like this:

module = importlib.import_module('sandwich.ham.ham')
ham_class = module.__dict__[module.__all__[0]]

If my analysis is correct, then we're facing a bit of a philosophical question. Your model of "mutation" is literally as if the source code itself had been modified, i.e. if CR actually went to the disk and edited source code files then this issue would never have come up. My approach, however - and it's pragmactic rather than principled - is to mutate at the "package" level, i.e. abstracted from source code. I think your interpretation is perhaps more pure.

@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2016

FWIW, I had a chance to try out the change to control.py that I suggested above, and it does seem to address the survivors. So I think I'm on the right track.

@atodorov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2016

@abingham - I've also tried importing the module as "sandwich.ham.ham" and it seems to work. However I've tried the same modification to my production software and I got the same results as before - mutation survived.

I also think that we're going to have the same problem even without modifying sys.path. What if some of my code base imports sandwich.ham.ham and another part (or tests) import it as ham. I will verify what is the behavior in this case but my guess is we're going to have similar results.

@atodorov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2016

@abingham - a simpler reproducer from sandwich.ham import ham, then tell CR to mutate sandwich.ham.ham, see atodorov/mutation-testing-example@8417663

Can we somehow load a mutated version of the module regardless of how exactly the SUT imports it ? I guess always convert to the full possible path. Any pointers where to look in CR's code ?

@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2016

I also think that we're going to have the same problem even without modifying sys.path.

You're right that, whether or not you modify sys.path, CR will not mutate a module that it hasn't been asked to mutate. So if you ask CR to mutate the "sandwich" (or whatever) package, but your always access the submodules of "sandwich" by other names, then your tests will always be using the unmutated versions of the modules.

@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2016

Can we somehow load a mutated version of the module regardless of how exactly the SUT imports it ? I guess always convert to the full possible path. Any pointers where to look in CR's code ?

Yes, that's the basic strategy I've been thinking of. When CR installs a mutant module, it would return that mutant for any module request which resolved to the same source file. I think this is doable, and actually pretty simple. I worry a bit about what we should do for modules that don't come from normal source files, though; that'll require some research.

The place to look is in cosmic_ray.importing.ASTFinder.find_spec(). This is where CR tells the import machinery what it can and can't manage imports for. I think we need to:

  1. Figure out how to find the full path to the module _fullname
  2. When the path argument to find_spec() matches the path in (1), return the configured loader.
@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2016

Looking at the new example you've posted, I don't see a reason why things shouldn't work. I'll try to look into it tomorrow.

@atodorov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2016

Just a quick FYI when the module is imported as import sandwich.ham.ham then find_spec is called with the following arguments:

++++++++ fullname=test_burger path=None
++++++++ fullname=test_control path=None
++++++++ fullname=sandwich.control path=['/home/atodorov/mutation-testing-example/sandwich']
++++++++ fullname=sandwich.ham.ham path=['/home/atodorov/mutation-testing-example/sandwich/ham']

and the mutation is killed.

When I use from sandwich.ham import ham then the log is

++++++++ fullname=test_burger path=None
++++++++ fullname=test_control path=None
++++++++ fullname=sandwich.control path=['/home/atodorov/mutation-testing-example/sandwich']

EDIT: looks like when del sys.modules['sandwich.ham.ham'] is executed we're left with sandwich and sandwich.ham references still available in sys.modules and that's why the second log above doesn't ask to import the module. If I manually delete sandwich.ham (hard-coded in CR code) from sys.modules after deleting sandwich.ham.ham then from sandwich.ham import ham triggers the parent module reload and we also load the mutant and the test fails.

NOTE: this is a slightly different issues than the original reported. For this I think it will suffice to traverse back and delete all the parent modules like sandwich.ham and sandwich.
In both cases self._fullname is sandwich.ham.ham

@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2016

OK, thanks for looking into this. I need to find some time to spend on this, just to convince myself I understand it all. There's definitely something unexpected going on, and you're probably on the right track...I just need to wrap my head around it before I'm comfortable merging this change in.

@abingham abingham closed this Jul 24, 2016

@atodorov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2016

@abingham - the original issue in comment #0 wasn't resolved by the latest patch so maybe reopen this issue ? I'll try to work on a reproducer test if I can.

@abingham abingham reopened this Jul 25, 2016

@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

I think I know how to address this, so I'll see if I can push up at least
an experimental patch today.

On Mon, Jul 25, 2016, 00:21 Alexander Todorov notifications@github.com
wrote:

@abingham https://github.com/abingham - the original issue in comment
#0 wasn't resolved by the latest patch so maybe reopen this issue ? I'll
try to work on a reproducer test if I can.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#157 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAE1eKqD_08K5rsUODsXrm9oFovxlgU6ks5qY-VcgaJpZM4JMfoE
.

@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

It doesn't look like the simple approach I was hoping for is going to work. I had misunderstood (really, misremembered) what the path argument to find_spec() was for. It's not the path to the source file as I was thinking...in retrospect, of course, that would be impossible for the import machinery to know at that point. So, back to square one.

The question we face with this bug is how we can know which module names are aliases. In your case you're expecting CR to know that 'sandwich.ham.ham' and 'ham' come from the same source file and to mutate them both. This is technically impossible for CR to know unless it somehow mimics the entire import facility. That is, "ham" and "sandwich.ham.ham" only resolve to the same source file under specific conditions, i.e. when the finders and loaders configured in sys.meta_path say they do. In principle, a loader could be installed which ignores sys.path (which you rely on to make "ham" and "sandwich.ham.ham" equivalent) and returns something else entirely for either or both of those module names. In other words, regardless of how you set sys.path, "ham" and "sandwich.ham.ham" are not universally the same.

Perhaps the right thing to do is to let users specify a list of aliases. This seems a bit clunky, though, and doesn't account for highly dynamic situations where the user might now know ahead of time how modules are being imported.

It might also be possible for us to hijack/replace the existing path-based finders and loaders. I'd have to do some reading (e.g. about path hooks) to really do this properly.

@atodorov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2016

Can we use module.__file__ or inspect.gesource() somehow ?

@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

Can we use module.file or inspect.gesource() somehow ?

Not with the current design, at least not easily (that I can see). The system basically works like this. We have a bit of code (the finder) installed on sys.meta_path. When you say "import foo", the finder gets asked "do you know how to import a module named 'foo'". CR's strategy is to install a finder which replied "yes" only for the particular module name which is currently being mutated; all other modules are handled by other finders.

Since the only piece of data the finder gets is the name of the module as a string, it doesn't have a module object or anything else to inspect. So it can't really figure out when "ham" and "sandwich.ham.ham" refer to the same thing. To determine that equivalence, we'd have to effectively run the import as if our finder wasn't in place, see how it resolved (i.e. to which file), undo the import, and then do the import with our finder in place. This approach isn't entirely out of the question, but it could be tricky; what worries me is that we'd be doing recursive imports in a manual way, and this feels hackish.

The approach I want to explore is something that feels cleaner to me. When a worker starts we can (and already do) figure out the path to the module which is going to be mutated...we do this by using the technique I described above, just not in a context where an import is already in progress. With this path in hand, we might be able to work in side the standard "path loading" algorithm to use our own loader when there is a path match, not just a name match. This is where I need to do some investigation.

@abingham

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

This should be fixed now. CR now mutates code on disk so that testing code interacts with mutated code just like it would in "the real world". Verifying this should be straightforward.

@abingham

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

This seems to be fixed now that we mutate files on disk.

Screenshot 2019-05-29 at 10 31 41

@abingham abingham closed this May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.