Port test suite from spec/nose to pytest(-relaxed)

[bitprophet]

Background

I've moved some of my other projects over to pytest by way of my heavily-inspired-by-spec pytest plugin, but left Invoke for close to last as it's got by the largest test suite of my projects at this point (given I wrote it myself & it's been TDD the whole way.)

Taking a quick stab at it now to see just how big a hurdle it is; if I'm lucky it'll mostly work as-is & I can switch all the tooling over, then port the idioms (eg replacing nose/spec/custom tests with assert, replacing some test helpers with fixtures as appropriate, etc) at whatever pace I want.

Mostly I just want to avoid having to write any additional code in the old vein, eg I want my fancy asserts (esp since I've gotten used to similar goodies in modern unittest like assertMultilineEqual.)

Also hoping for maybe a bit more speed, but suspect the now 5 full seconds long test run (!) is probably more due to hundreds of file accesses (loading configs and tasks fixtures) and subprocess overhead, than to nose being "slow".

TODO

• Initial base pass re: getting all tests passing, period, as-is or as close as possible
• Uninstall spec, remove all spec imports, and replace use of its tooling (eg assert_contains etc)
• Consider replacing the non-spec-related parts of IntegrationSpec (mostly setup/teardown) with pytest class-level fixtures...but punting's OK too (maybe make a spinoff ticket)
• Integration tests
• Coverage execution
• Python 3 checkup.
• Release some more tweaks to pytest-relaxed I had to make
• Update our requirements to use that version
• Triple check same test count
• Update internal Invoke test tasks (should be able to use invocations as elsewhere)
• Make sure any missing-but-acceptable functionality is documented over on relaxed's tracker (e.g. lack of docstring support)

[bitprophet]

Legacy run on latest master yields this:

Ran 818 tests in 4.878 seconds
OK (16 skipped)

Once that same number of tests runs under pytest, this is technically done.

[bitprophet]

Collection-level errors!

___________________________________________________________ ERROR collecting tests/platform.py ___________________________________________________________
import file mismatch:
imported module 'platform' has this __file__ attribute:
  /usr/local/var/pyenv/versions/2.7.13/lib/python2.7/platform.py
which is not the same as the test file we want to collect:
  /Users/jforcier/Code/oss/invoke/tests/platform.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules
________________________________________________________ ERROR collecting tests/parser/context.py ________________________________________________________
import file mismatch:
imported module 'context' has this __file__ attribute:
  /Users/jforcier/Code/oss/invoke/tests/context.py
which is not the same as the test file we want to collect:
  /Users/jforcier/Code/oss/invoke/tests/parser/context.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules

Also of note, these actually show up twice. Not sure if that's pytest-relaxed's fault or not, Invoke's suite is already spec/relaxed style in layout, so running vanilla pytest is of course a lot noisier. Aside from potential "why is collection running multiple times?" speed concerns, tho, treating this as just 2 issues.

The parser/context one is irritating though since having some minor module/package nesting is a pattern I sort of like these days and it seems kinda dumb of pytest to get confused by it. Time to dig in.

[bitprophet]

Seems pytest tries importing all tests via sys.path munging by default and the way to work around this is to just make the test packages actual packages (i.e. rub __init__.py on it).

The discussion I found implied one has to do this to the tests dir too, which then leads to issues with overall importing if one ever wants to test an externally-installed copy of the code instead of the code next to the tests (which in turn leads to the not-uncommon-but-I-personally-find-it-ugly src/theactualcodehere pattern).

However, I only added __init__.py to my parser subdir and that seems to have been enough to remove the confusion...for now.

re: platform.py: think we can chalk that up to a legitimate Bad Naming Idea and just rename it; just because I personally haven't had it cause an import related issue doesn't mean it won't for others or in future.

[bitprophet]

With those out of the way, it's on to other stuff like dealing with more of pytest's sys.path munging causing some other silly issues with test fixture loading.

For example, I have a decorator.py in my test fixtures. Used to load fine when I put my fixtures folder at 0th slot in sys.path. Now, the decorator pip-installed 3rd party lib is showing up instead. AFAIK this only happens if something else already did import decorator and thus "polluted" sys.modules. Given pytest has a very different collection setup from nose, this is likely.

[bitprophet]

Latest bizarre hang-up, one of my test classes extends IntegrationSpec's setup and of course calls super to do so. Works great under nose. Blows up under pytest with the usual super error, even tho the objects in play appear the same in both cases:

self = <executor.Executor_ object at 0x105c91390>

    def setup(self):
        print(Executor_)
>       s = super(Executor_, self)
E       TypeError: super(type, obj): obj must be an instance or subtype of type

../executor.py:13: TypeError
----------------------------------------------------------------- Captured stdout setup ------------------------------------------------------
<class 'executor.Executor_'>
<class 'executor.Executor_'>

Not sure what the fuck. I do note that pytest is loading the setup twice for some reason. If I remove Spec from IntegrationSpec (just make it inherit from object) one of those two print lines goes away, but otherwise, no dice. Calling IntegrationSpec.setup(self) instead gets a different TypeError:

TypeError: unbound method setup() must be called with IntegrationSpec instance as first argument (got init instance instead)

[bitprophet]

Havin a real weird hangup with some of my most complex tests/test setups, where pytest is erroneously getting mad about a "missing fixture" arg:

• Runner test suite
• @mock_pty, which has an insert_os kwarg controlling whether or not the decorator injects a mock OS module into the test's args, appears to work fine
• @mock_subprocess has a similar kwarg, insert_Popen, but whenever it's in play, pytest thinks the mock_Popen arg in the test function is referencing a fixture and explodes.
  • But only those tests using insert_Popen=True, of course; the ones not using it have no extra args so things are fine.
• I see no obvious differences between the two decorators besides the fact that @mock_pty has some @patch decorators around its inner function wrapper, and @mock_subprocess does not. Guessing this may be altering the signature exposed to pytest and thus confusing the fixture-arg magic?
  • What's odd is both wrappers have @wraps around them at the topmost level, so they ought to be exposing the test function's signature, which is similar in both cases.

[bitprophet]

Seems the crux is pytest.compat.getfuncargnames which I note, not having stepped thru yet, does a lot of attempts to introspect Mock-related magic...my pytest is currently 3.2.5 (3.3 breaks other parts of -relaxed and I haven't had time to fix yet) so it's this: https://github.com/pytest-dev/pytest/blob/a220a40350a8a81c0f0e1986aa184e4c33da29d6/_pytest/compat.py#L86-L119

(of note, master has made a lot of changes to this function, tho the little note in docstring is still there. most of the changes appear to have come from pytest-dev/pytest#2842)

Further digging finds pytest-dev/pytest#2828 which seems like it almost perfectly describes the problem. Upshot's not clear though, do I need to be doing some weird shit with this __signature__ thing?

Feels like better is to just dig thru how this func is breaking me in the one case but not the other, and see if I can just apply the incidental "fix" from the good case. Eg some no-op @patch or something.

[bitprophet]

Yea, it's exactly what I thought, the code explicitly "skips" ahead going by number of mock patch wrappers, only grabbing the remainder as potential fixtures. Sure enough, adding a "doesn't touch anything I care about" patch decorator (including the usual bookkeeping of popping it back out of the args list) gets things working.

BLARGH.

[bitprophet]

Also of note, I would have preferred to fix this by just switching to whatever pytest's idiomatic solution is for this sort of "fixture" - except I don't recall if there are any good ways to parameterize fixture use and these decorators are heavily parameterized. Whether this was actually faster than that, IDK, but it's done for now.

[bitprophet]

All modules appear to pass individually with the right number of tests

Sadly running 'em all at once still has some issues, the tests/tasks.py file explodes (and then it + the few after it don't get counted either, leaving us with about 200 missing tests, heh.)

INTERNALERROR>   File "/Users/jforcier/.virtualenvs/invoke/lib/python2.7/site-packages/_pytest/runner.py", line 61, in pytest_runtest_protocol
INTERNALERROR>     nodeid=item.nodeid, location=item.location,
INTERNALERROR>   File "/Users/jforcier/.virtualenvs/invoke/lib/python2.7/site-packages/_pytest/main.py", line 580, in location
INTERNALERROR>     location = self.reportinfo()
INTERNALERROR>   File "/Users/jforcier/.virtualenvs/invoke/lib/python2.7/site-packages/_pytest/python.py", line 249, in reportinfo
INTERNALERROR>     fspath = sys.modules[obj.__module__].__file__
INTERNALERROR> KeyError: 'tasks'

Given it's happening on tasks.py I wonder if this is pytest combining with our own tasks file loading behavior to cause some sort of issue with sys.modules...digging.

---

Also, global collection skips tests/parser/* which may be pytest's fault or may be relaxed's fault, not sure yet.

[bitprophet]

Narrowed it down to running the program suite before tasks. No other suites matter; as soon as program is dropped, or moved after tasks, or tasks is dropped – error goes away. Clear state bleed problem triggering an issue in the tasks suite somehow (or rather, in the reporting of its results.) Just not sure what and as usual why it wasn't an issue with nose.

[bitprophet]

Smells like something to do with how pytest mucks around with sys.modules (eg another earlier issue was vaguely similar) and rather than go down another hour+ long rabbit hole, I think I'm just gonna rename the test suite file - it's always bothered me anyways that it "looks" a lot like it's a sub-tasks file that just happens to be in the test suite.

[bitprophet]

Just renamed it task from tasks...not ideal (I prefer them to match the source file names) but whatever. ZoP #9 and all that.

With that...I think phase 1 complete? 802 passed, 16 skipped in 4.38 seconds. (Note that nose includes skipped in its total count, and pytest does not, so both total 818 tests.)

Also, it was finding the parser tests after all, they just got sorted after the report-breaking tasks and thus did not appear.

[bitprophet]

Oh, huh. Part of the issue may have just been that IntegrationSpec was no longer applied everywhere (in fact...yes! I had to remove it from the Program tests for other reasons during this work!) and it has one thing I didn't make a fixture yet - cleaning of sys.modules to "unload" all the task-module data-fixtures.

Except - no, reinstating it doesn't make a difference, and the problem is that something is being unloaded too eagerly from sys.modules - the inverse of what my stuff did / does.

Meh, at least it's there now.

[bitprophet]

Now cleaning up a lot of expect() (test helper) use cases around using nose's assert_contains and co.

Interestingly, from "Jeff in 2018" perspective, I realize that this case for expect - passing in both the test function and one of the two comparison operands - is a pretty gross way to do things, compared to simply returning the captured stdout (the "hard" part of expect) and doing an assert (or, to the point, even an assert_contains.)

So I've factored out the run+capture part of expect into a local run() and now not only is the code becoming less tied to nose/spec, it's just straight up cleaner/shorter!

Leaving the "base" case of expect alone for now as it's less problematic & far more widespread (aka more work to replace.)

[bitprophet]

Seems like test suite still Just Works under Python 3. Not a big surprise given all the search and replaces were for syntax that isn't Python 3 sensitive.

One weird thing I noticed is that under Python 3, coverage is way under-reporting for some reason, it's only hitting the __init__s, _version and main (vs, well, every single module under Python 2.) The test output within coverage shows it running all tests/test modules though.

Went back to master + having spec installed, and uh, coverage doesn't even work there under Python 3 (wacky KeyErrors about chunks?) so I suppose my earlier guess that "we never run coverage under Python 3 anyways" is accurate? Not ideal but not a blocker.

[bitprophet]

A big ol' flake8 pass and woof, I think this is done. Feels good. EDIT: well, still gotta make sure Travis is cool with it.

[bitprophet]

Oh hm wait Travis would possibly have been running coverage under Python 3 (insofar as the Travis config's default "run the actual tests" is usually coverage style), and for sure it is doing so now and kabooming.

Seems like it did work OK under nose: https://travis-ci.org/pyinvoke/invoke/jobs/357642954#L559

[bitprophet]

On latest pytest-cov so that's set already. Haven't doublechecked its deptree yet.

Only thing I'm finding right now on pytest-cov's tracker is pytest-dev/pytest-cov#196 (which implies it's travis-specific - something I doubt since I am getting the same symptom locally myself!)

It notes rightly that the traceback fingers coverage itself as the culprit. Mine's...yea it's old. Updating it + some of its deps seems to fix it on my machine.

[bitprophet]

Yup that did it. Toot toot. https://travis-ci.org/pyinvoke/invoke/builds/367423555