Pytest and CI

[Peque]

This pull request implements initial pytest compatibility and CI with Travis. See the results after setting up the repository with Travis.

Tests are executed with tox, meaning that the user can very easily reproduce all test cases (with different Python interpreters) locally and easily. tox calls pytest to run those test files that have been adapted. Then, the rest of the files, are tested as before (simply executing the test file with the interpreter).

It is a step forward to CI and pytest integration. Although it is not complete, I think it could be merged as-is (I don't see any downsides).

Locally, I have already adapted most of the test files to be pytest compatible. However, they all fail at least at one point. Therefore, I would need more time to fix them and, probably, discuss them with you @mmckerns to understand what the test is doing and why is now failing.

Any comments/suggestions?

[Peque]

Related issues/pull requests: #3, #139, #162, #191, #198

Also #114 (just for the inclusion of a MANIFEST.in file, which was necessary to run the tests).

[matsjoyce]

Looks good 👍 . What we could do it merge this into a separate branch, iron out the bugs there first, and then merge it into master which will stop the false negatives for other pull requests while we work on this.

[Peque]

@matsjoyce As it is right now it should not generate false negatives (the bugs are just with the changes I have locally in my computer), so maybe it could directly be merged to master as a step forward?

Even if it stays like this forever (thinking about the worst case, in which we can not deal with the bugs when adapting the rest of the files), I think it is better this way. What you think?

[matsjoyce]

Getting CI can only be a good thing unless it generates tons of spam, which travis shouldn't. I think we should merge ASAP, but it's not my call.

[mmckerns]

This looks good. As I mentioned in #191, dill needs to be using CI -- I wanted to be using it already a few years ago, and just have not gotten around to it. So I will review this and #191, and see if we can PR one or both.

[Peque]

@mmckerns Thanks for considering this pull request.

It has few differences with respect to #191:

• It is an attempt not only to enable Travis CI, but to refactor/split some tests to make their output more useful.
• It also uses tox to allow the user (not only Travis) to run all tests locally with a single command and against multiple interpreters.
• As in Enable Travis CI #191, it also runs the tests that have not been refactored/split yet "the old way" (executing the files with the python interpreter). However, I think Enable Travis CI #191 will stop when the first error occurs, while this implementation continues until the end (while still reporting a fail at the end if it happened, of course). Maybe @kernc can confirm this, as I may be mistaken.

[mmckerns]

@Peque: Yes, I see the differences -- but thanks for itemizing them in a compact way.

[mmckerns]

I'm going to start with the PR in #191. I like a lot of what is in your PR, and I think a lot of it should be used in dill. I'm going to test and explore a little more, then get back to you on this PR.

[mmckerns]

don't move check from dill to test_check. Otherwise, the change to test_check looks good.

[mmckerns]

this change makes pytest required to run the tests. I'd rather not do that. Otherwise, the changes to test_file are fine.

[mmckerns]

ultimately I think your tox.ini and travis.yml are a better option than PR #191, and is a fine start for adding tox and pytest support. I also see the test failures with pytest and need to better understand why. I'll come back to approving your tox.ini and travis.yml later.

[mmckerns]

Why run with pytest at all if you are going to run the tests with bash?

[mmckerns]

dill works for 2.6-2.7 and 3.1-3.6 and pypy and pypy3, with some minor errors (as seen in test suites). Do you mean "unsupported" by there is a single test failure, or otherwise?

[mmckerns]

@Peque: can you move the conflicting travis.yml into a separate PR, so we split this into a PR for (1) using pytest, and (2) hooking pytest into travis (I assume with tox)? Then I think we can get the pytest stuff pulled pretty quickly.

[mmckerns]

See comments and change requests in the comments above.

[Peque]

@mmckerns Thanks for taking the time to review the changes. Next time, if you can, add your comments associated to the specific changes (i.e.: line of code or file), because that way it is easier for me to know to which code part you are referring to (I think you just clicked on the pull request's "global" Review Changes button).

don't move check from dill to test_check. Otherwise, the change to test_check looks good.

Ok, just thought it was really a test-only function, which didn't make sense as actual dill code.

this change makes pytest required to run the tests. I'd rather not do that. Otherwise, the changes to test_file are fine.

I understand you would want, at first, to avoid that. However, pytest has many "nice" things that could help make other tests much nicer, so I would vote (if I may) for adding it as the official tool to run dill tests. Really many projects now are using pytest as the "de facto" testing tool nowadays.

ultimately I think your tox.ini and travis.yml are a better option than PR #191, and is a fine start for adding tox and pytest support. I also see the test failures with pytest and need to better understand why. I'll come back to approving your tox.ini and travis.yml later.

If you like tox, I can easily create a pull request for that only. I think it is a good change anyway. It allows you to run all the tests using multiple interpreters locally (then we can discuss adding the pytest stuff in another pull request).

Why run with pytest at all if you are going to run the tests with bash?

Bash runs only old tests (new modified tests do not run any code when executed with python testX.py). New tests only have functions defining each separate test. So it is a way to make sure all tests are run (when possible, using pytest, otherwise, running them with the interpreter).

dill works for 2.6-2.7 and 3.1-3.6 and pypy and pypy3, with some minor errors (as seen in test suites). Do you mean "unsupported" by there is a single test failure, or otherwise?

I think adding tox support for now may help you run the tests locally. As far as I have seen (and Travis seems to agree with me), Python 3.5, 3.6 and pypy (2 and 3) cannot run the test suite without errors. Are you able to run the tests with those interpreters locally?

@Peque: can you move the conflicting travis.yml into a separate PR, so we split this into a PR for (1) using pytest, and (2) hooking pytest into travis (I assume with tox)? Then I think we can get the pytest stuff pulled pretty quickly.

If you agree, as I have mentioned, we could start by simply integrating tox. Then we can start migrating old tests to pytest and discuss the errors that the migration may raise (I could create another pull request for that).

[mmckerns]

@Peque: yeah sorry about the dissociated comments, I was trying something new and it didn't work as I expected.

[Peque]

@mmckerns Notice that, when you hover your mouse on the code (on the "Files Changes" tab of the pull request), there is a + sign that appears and allows you to add a comment there. That may work better. 😊

[mmckerns]

I understand you would want, at first, to avoid that. However, pytest has many "nice" things that could help make other tests much nicer, so I would vote (if I may) for adding it as the official tool to run dill tests. Really many projects now are using pytest as the "de facto" testing tool nowadays.

Yes, I know… but bear with me. I'm going to use pytest, but not immediately. Maybe in a day or two, when I get all of this sorted. So, if you wouldn't mind, let's split the PR into separate PRs. I'd like one PR that changes the test code to make it more pytest friendly (without requiring pytest), and one PR for tox only is fine, and then I think what is left is the travis.yml file, and potentially baking in pytest.

I would love to use pytest for travis… but not to force someone who wants to try the tests to install pytest. I've used pytest before several times on other projects, and it's nice, yes.

Bash runs only old tests (new modified tests do not run any code when executed with python testX.py). New tests only have functions defining each separate test. So it is a way to make sure all tests are run (when possible, using pytest, otherwise, running them with the interpreter).

Aha, I didn't register that you had deleted the __main__ block. I see, yeah. Duh.

I think adding tox support for now may help you run the tests locally. As far as I have seen (and Travis seems to agree with me), Python 3.5, 3.6 and pypy (2 and 3) cannot run the test suite without errors. Are you able to run the tests with those interpreters locally?

The tests pass locally for 3.5 and 3.6, with the exception of one test (namedtuple) in test_classdef, and there's a ticket for that #132, and also pypy has only one test fail dill_bugs (with issue #175). So, I see exactly what travis sees.

[Peque]

Closed this in favor of #199 and #200.

[mmckerns]

thanks