Tarmac

Bug #1258248
Comment #2

Comment 2 for bug 1258248

Revision history for this message

David Britton (dpb) wrote on 2014-02-27:

Hi --

I did a lot of investigation over the past 2 days and tracked this down to a module import isolation problem with the unittest runner. If you have an environment to reproduce the issue (for me trusty still hit it), running with:

nosetests tarmac

trial tarmac

would not hit the problem. Also, running just the failing tests in isolation does not hit the problem. For example:

python -m unittest tarmac.plugins.tests.test_command.TestCommand #PASS
python -m unittest discover #FAIL

This led me to find the other test (or tests) that were causing an issue. I eventually narrowed it down to the mocking library's "@patch" decorator, not working for the plugin that was under test (tarmac.plugins.command). Which led me to the plugin.load_plugins() test cases. One of those tests cases (test_load_plugins) actually did a delattr() on all loaded plugins both before the test, and after the test had executed.

I didn't root cause why `python -m unittest discover` hits this issue where trial and nose do not, but I chalked it up to better isolation native to those runners, which I've experienced in other various bugs on other projects. They also both publish that they have better isolation features.

The only other avenue I walked down was looking into python-mock. the `@patch` decorator claims to only make the patch exactly before the test runs, and revert it after, but perhaps the symbol table it's using is somehow corrupted by the `delattr()` call in the load_plugin() test. Anyway, checking into bugs in python-mock didn't prove fruitful.

The attached branch should solve the issue. It slightly changes test coverage in that it doesn't explicitly load each plugin, but that is taken care of in other places in the tests already (thus the reason it had to delattr(), they were already loaded). I think isolating it to explicitly load a dummy plugin is more in line with the intent of the unit test anyway. Given others were already doing that in the suite, I trust this should be uncontroversial to accept.

Hi --

I did a lot of investigation over the past 2 days and tracked this down to a module import isolation problem with the unittest runner.  If you have an environment to reproduce the issue (for me trusty still hit it), running with:

nosetests tarmac

trial tarmac

would not hit the problem.  Also, running just the failing tests in isolation does not hit the problem.  For example:

python -m unittest tarmac.plugins.tests.test_command.TestCommand  #PASS
  python -m unittest discover #FAIL

This led me to find the other test (or tests) that were causing an issue.  I eventually narrowed it down to the mocking library's "@patch" decorator, not working for the plugin that was under test (tarmac.plugins.command).  Which led me to the plugin.load_plugins() test cases.  One of those tests cases (test_load_plugins) actually did a delattr() on all loaded plugins both before the test, and after the test had executed.

I didn't root cause why `python -m unittest discover` hits this issue where trial and nose do not, but I chalked it up to better isolation native to those runners, which I've experienced in other various bugs on other projects.  They also both publish that they have better isolation features.

The only other avenue I walked down was looking into python-mock.  the `@patch` decorator claims to only make the patch exactly before the test runs, and revert it after, but perhaps the symbol table it's using is somehow corrupted by the `delattr()` call in the load_plugin() test.  Anyway, checking into bugs in python-mock didn't prove fruitful.

The attached branch should solve the issue.  It slightly changes test coverage in that it doesn't explicitly load each plugin, but that is taken care of in other places in the tests already (thus the reason it had to delattr(), they were already loaded).  I think isolating it to explicitly load a dummy plugin is more in line with the intent of the unit test anyway.  Given others were already doing that in the suite, I trust this should be uncontroversial to accept.