Python test harness #120
Python test harness #120
Conversation
| - | ||
| language: python | ||
| python: | ||
| - "3.6" |
There was a problem hiding this comment.
Was there no 3.7 on Travis, or were you using this to make sure not-quite-newest Pythons work?
There was a problem hiding this comment.
You have to request a specific distribution to get 3.7: https://docs.travis-ci.com/user/languages/python/#python-37-and-higher. If you're cool with specifying the distribution (I am), we can do 3.7. I would like to keep 3.6 as an additional job though since Python versioning has already bitten us recently with fathom-serve.
There was a problem hiding this comment.
I'm loath to delay landing this important PR any longer than absolutely necessary, so let's leave it as is and add 3.7 later, as a separate PR, if it gives us anything. If we have to be 3.6-compatible anyway (which I agree we seem to have to, practically), I don't see any benefit to testing on 3.7 at all—breaking changes are rare in CPython.
There was a problem hiding this comment.
That is a good point about the lack of benefit from testing on 3.7 if we're testing on 3.6. I'll remove the 3.7 job.
There was a problem hiding this comment.
And now I see you went ahead and added it, and it works. Great. I don't even care if it adds anything, because I'm so psyched to get this patch landed. :-)
There was a problem hiding this comment.
Haha, okay. It'll stay!
.travis.yml
Outdated
| python: | ||
| - "3.6" | ||
| install: | ||
| - pip install -r cli/test_requirements.txt |
There was a problem hiding this comment.
What I've always done is let the requirements live in setup.py (where they have to be anyway) and kick off the testrunner with python setup.py test, which installs the deps and kicks off the tests in one step. (There's a little config you have to do in setup.py to make that work; I'll post here if I find it.) That (1) keeps us from repeating ourselves, (2) tests that the requirements specified in setup.py work, and (3) saves us manually doing the install here. Whaddaya think?
--
Hmm, on further reading, I find a disquieting amount of trickery necessary to do this. setup_requires—eww. Maybe it's good enough to just not repeat ourselves: pip install ..
I do wonder: where is the copy of pytest we're using coming from? I don't see it in tests_require. It makes me nervous to rely on it just happening to be around on the Travis box—but only a little.
There was a problem hiding this comment.
I would have liked to keep all the requirements in one place, but had seen similar trickery required when I looked into it.
That's a good point about the pytest version. I'll put it explicitly in test_requirements.txt
There was a problem hiding this comment.
I just noticed in the build output that the image comes with pytest:
platform linux -- Python 3.7.1, pytest-4.6.3, py-1.8.0, pluggy-0.12.0
There was a problem hiding this comment.
Is there a reason we can't keep the reqs in setup.py and just run pip install -e . in the cli folder as a pre-step? I'm allergic to the repetition (I, for one, am constantly going to forget to update test_requirements.txt), and I'm prepared to let the pytest version be unspecified for the moment.
There was a problem hiding this comment.
That does indeed work. Sorry for the confusion!
* I will need to revert some changes after testing that this works
|
np!
|
|
@erikrose Anything else you want to see here? |
|
Nope, ship it! |
Addresses #114.
This PR adds the Makefile and CI infrastructure for testing our Python code. This PR also adds testing for one methods in fathom-test to get the ball rolling.
When this PR is merged, I will open an issue for each tool to write tests for that tool.