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

Test with pytest #2815

Closed
wants to merge 16 commits into from
Closed

Test with pytest #2815

wants to merge 16 commits into from

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Oct 26, 2017

Fixes #2812.

Changes proposed in this pull request:

  • Use pytest for testing instead of nose. So far only on Travis/Linux builds.

Comparing a before and after coverage report shows the coverage dropped, and that's a good thing! Nose was missing out 4 source files as they weren't touched at all by tests: GdImageFile.py, ImageDraw2.py, OleFileIO.py, WalImageFile.py. Otherwise the Python and C coverage is identical.

framework Files Tracked lines Lines hit Lines partials Lines missed Coverage
nose 165 21,708 18,098 0 3,610 83.37%
pytest 169 21,839 18,098 0 3,741 82.87%

TODO:

  • Update Windows: appveyor.yml, winbuild/appveyor_install_msys2_deps.sh, winbuild/build.py
  • Update helper.py: there's some nose-specific logic in helper.py to retain temp files when a test fails. Is this still necessary?
  • Update helper.py: remove shortDescription which prevented printing docstrings in nosetests
  • Update docker-images
  • Update pillow-wheels
  • Add parallel testing
  • Update commands in setup.py
  • Update commands in Tests/README.rst
  • Update test-installed.py, profile-installed.py

@hugovk
Copy link
Member Author

hugovk commented Oct 29, 2017

Updated AppVeyor/Windows builds with pytest, now with codecov coverage!

Coverage goes up from 82.87% to 83.19%.

https://codecov.io/gh/hugovk/pillow/tree/ccbb85794c1399efea5b87a49b82309d4f1cb0e0/PIL
https://codecov.io/gh/hugovk/pillow/tree/6c1428b4c188fb67a75f1415850a77449076ef7c/PIL

@hugovk
Copy link
Member Author

hugovk commented Nov 6, 2017

test-installed.py

@wiredfool Do you still use test-installed.py? Please can you check if this pytest version works for you? (I've commented out the numprocess bit because it causes a test to fail, will check this later.)

profile-installed.py

And do you still use profile-installed.py? I've not updated it, but running it on master with no changes gives a NameError:

⌂93% [hugo:~/github/Pillow] master(5)+* ± python profile-installed.py
         4 function calls in 0.001 seconds

   Ordered by: cumulative time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.001    0.001 profile:0(nose.main())
        1    0.001    0.001    0.001    0.001 :0(setprofile)
        1    0.000    0.000    0.000    0.000 :0(exec)
        1    0.000    0.000    0.000    0.000 <string>:1(<module>)
        0    0.000             0.000          profile:0(profiler)


Traceback (most recent call last):
  File "profile-installed.py", line 25, in <module>
    profile.run("nose.main()", sort=2)
  File "/usr/local/Cellar/python3/3.6.3/Frameworks/Python.framework/Versions/3.6/lib/python3.6/profile.py", line 93, in run
    return _Utils(Profile).run(statement, filename, sort)
  File "/usr/local/Cellar/python3/3.6.3/Frameworks/Python.framework/Versions/3.6/lib/python3.6/profile.py", line 55, in run
    prof.run(statement)
  File "/usr/local/Cellar/python3/3.6.3/Frameworks/Python.framework/Versions/3.6/lib/python3.6/profile.py", line 418, in run
    return self.runctx(cmd, dict, dict)
  File "/usr/local/Cellar/python3/3.6.3/Frameworks/Python.framework/Versions/3.6/lib/python3.6/profile.py", line 424, in runctx
    exec(cmd, globals, locals)
  File "<string>", line 1, in <module>
NameError: name 'nose' is not defined

@wiredfool
Copy link
Member

Can't say that I remember ever using profile-installed. I do use test-installed.py all the time, as I really don't like the in-place build thing. (fwiw, my usual loop is make install && make test, of course after sourcing an appropriate virtualenv)

@wiredfool
Copy link
Member

wiredfool commented Nov 8, 2017

I've been attempting to fix the coverage for the Docker images (which are only sort of catching the C code currently) by following the changes here, but it's not working.

  • Running coverage run test-installed.py doesn't find anything to cover, with many different ways of attempting to invoke it.
  • Similar with pytest -vx --cov PIL --cov-report xml Tests
  • Running python setup.py test does cover the python, but not the C, which it installs in-place.

@hugovk
Copy link
Member Author

hugovk commented Nov 15, 2017

test-installed.py

Trying to get test-installed.py working:

  • Build with python2 setup.py install -- fine
  • Test with python2 test-installed.py -- not fine

The path-monkeying inside test-installed.py:

del(sys.path[0])
sys.path.insert(0, os.path.abspath('./Tests'))

is working and removing /Users/hugo/github/Pillow' and inserting ['/Users/hugo/github/Pillow/Tests', ....

But pytest (when called as pytest.main()) is re-inserting the current directory. When doing this at the top of test_000_sanity.py:

print(os.path.dirname(PIL.__file__))
print(sys.path)

We get PIL from the wrong place due to the path:

/Users/hugo/github/Pillow/PIL
['/Users/hugo/github/Pillow/Tests', '/Users/hugo/github/Pillow', '', ...

However, running a simplepytest on its own has a good path inside test_000_sanity.py:

/usr/local/lib/python2.7/site-packages/Pillow-4.4.0.dev0-py2.7-macosx-10.12-x86_64.egg/PIL
['/Users/hugo/github/Pillow/Tests', '/usr/local/bin', ...

I think that is explained by this -- (https://docs.pytest.org/en/latest/pythonpath.html#invoking-pytest-versus-python-m-pytest); calling pytest.main() is like doing python -m pytest:

Running pytest with python -m pytest [...] instead of pytest [...] yields nearly equivalent behaviour, except that the former call will add the current directory to sys.path.

How about we remove test-installed.py and update the Makefile so test: just calls pytest -qq?

@hugovk
Copy link
Member Author

hugovk commented Dec 9, 2017

Python 3.3 is failing on AppVeyor because pytest no longer supports EOL Python 2.6 or 3.3.

%PYTHON%\%PIP_DIR%\pip.exe install pytest pytest-cov
Collecting pytest
  Downloading pytest-3.3.1-py2.py3-none-any.whl (184kB)
pytest requires Python '>=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*' but the running Python is 3.3.5
Command exited with code 1

We could pin to an old version of pytest for 3.3, but I'd rather we just drop it to make things easier for us, as I expect pytest won't be the only thing dropping 3.3 in the near future.

Here's the pip installs for pillow from PyPI for the last month (via pypinfo --percent --pip pillow pyversion).

python_version percent download_count
2.7 67.1% 908,968
3.6 15.5% 210,684
3.5 12.2% 165,583
3.4 4.8% 64,461
2.6 0.2% 2,607
3.7 0.1% 1,985
3.3 0.1% 1,101
3.2 0.0% 63
None 0.0% 13

Thoughts?

@wiredfool
Copy link
Member

If we drop a python version, we've got to have it in the release notes, and I think we're going to trigger a major version rev as well.

@hugovk
Copy link
Member Author

hugovk commented Dec 13, 2017

For parallel testing, there's a problem with a test:

pip install pytest-xdist
pytest -n 4
...
============================================================================ FAILURES =============================================================================
___________________________________________________________________ TestFileTiff.test_gimp_tiff ___________________________________________________________________
[gw1] darwin -- Python 3.6.3 /usr/local/opt/python3/bin/python3.6

self = <test_file_tiff.TestFileTiff testMethod=test_gimp_tiff>

    def test_gimp_tiff(self):
        # Read TIFF JPEG images from GIMP [@PIL168]

        codecs = dir(Image.core)
        if "jpeg_decoder" not in codecs:
            self.skipTest("jpeg support not available")

        filename = "Tests/images/pil168.tif"
        im = Image.open(filename)

        self.assertEqual(im.mode, "RGB")
        self.assertEqual(im.size, (256, 256))
        self.assertEqual(
            im.tile, [
                ('jpeg', (0, 0, 256, 64), 8, ('RGB', '')),
                ('jpeg', (0, 64, 256, 128), 1215, ('RGB', '')),
                ('jpeg', (0, 128, 256, 192), 2550, ('RGB', '')),
>               ('jpeg', (0, 192, 256, 256), 3890, ('RGB', '')),
                ])
E       AssertionError: Lists differ: [('jpeg', (0, 0, 256, 256), 0, ('RGB', 'jpeg', False))] != [('jpeg', (0, 0, 256, 64), 8, ('RGB', '')), ('jpeg', (0, 64, 25[121 chars]''))]
E
E       First differing element 0:
E       ('jpeg', (0, 0, 256, 256), 0, ('RGB', 'jpeg', False))
E       ('jpeg', (0, 0, 256, 64), 8, ('RGB', ''))
E
E       Second list contains 3 additional elements.
E       First extra element 1:
E       ('jpeg', (0, 64, 256, 128), 1215, ('RGB', ''))
E
E       - [('jpeg', (0, 0, 256, 256), 0, ('RGB', 'jpeg', False))]
E       ?                       --    ^           ---- -------  ^
E
E       + [('jpeg', (0, 0, 256, 64), 8, ('RGB', '')),
E       ?                        +   ^              ^
E
E       +  ('jpeg', (0, 64, 256, 128), 1215, ('RGB', '')),
E       +  ('jpeg', (0, 128, 256, 192), 2550, ('RGB', '')),
E       +  ('jpeg', (0, 192, 256, 256), 3890, ('RGB', ''))]

Tests/test_file_tiff.py:87: AssertionError

This doesn't happen with plain sequential pytest, or with pip install pytest-randomly to randomise the order (unless you're unlucky).

@wiredfool
Copy link
Member

That test is changing shortly anyway.

@wiredfool
Copy link
Member

We're ~10 days out, do you think this one is going to land?

@hugovk
Copy link
Member Author

hugovk commented Dec 20, 2017

Yes. I think we're more or less good in this repo and it's ready for review. The parallel things can come later.

Next steps is to update docker-images and pillow-wheels. I'm not sure what's the best way to tackle those, regarding the dependencies between those repos and this. Any ideas?

@hugovk hugovk changed the title [WIP] Test with pytest Test with pytest Dec 20, 2017
@wiredfool
Copy link
Member

Looks like the removal of scripts caused a merge error here.

@hugovk
Copy link
Member Author

hugovk commented Dec 27, 2017

  • Testing an individual file isn't working either, it's running the whole test suite when I try to run pytest -qq Tests/test_file_png.py

Try pytest [-qq] -k Tests/test_file_png.py, it's working for me.

@wiredfool
Copy link
Member

Ah, -k.

Also, can we remove the --cov from the default options, as there's no need to run coverage on dev machines.

@wiredfool
Copy link
Member

wiredfool commented Dec 27, 2017

So, pytest-xdist definitely has a different path when running with -n auto:

Multiproc:

['/Users/erics/Pillow/Tests', '/Users/erics/Pillow', '', '/Users/erics/vpy35/lib/python3.5/site-packages/olefile-0.43-py3.5.egg', '/Users/erics/vpy35/lib/python3.5/site-packages/Pillow-5.0.0.dev0-py3.5-macosx-10.11-x86_64.egg', '/Users/erics/vpy35/lib/python35.zip', '/Users/erics/vpy35/lib/python3.5', '/Users/erics/vpy35/lib/python3.5/plat-darwin', '/Users/erics/vpy35/lib/python3.5/lib-dynload', '/usr/local/Cellar/python3/3.5.1/Frameworks/Python.framework/Versions/3.5/lib/python3.5', '/usr/local/Cellar/python3/3.5.1/Frameworks/Python.framework/Versions/3.5/lib/python3.5/plat-darwin', '/Users/erics/vpy35/lib/python3.5/site-packages']

vs:

['/Users/erics/Pillow/Tests', '/Users/erics/vpy35/bin', '/Users/erics/vpy35/lib/python3.5/site-packages/olefile-0.43-py3.5.egg', '/Users/erics/vpy35/lib/python3.5/site-packages/Pillow-5.0.0.dev0-py3.5-macosx-10.11-x86_64.egg', '/Users/erics/vpy35/lib/python35.zip', '/Users/erics/vpy35/lib/python3.5', '/Users/erics/vpy35/lib/python3.5/plat-darwin', '/Users/erics/vpy35/lib/python3.5/lib-dynload', '/usr/local/Cellar/python3/3.5.1/Frameworks/Python.framework/Versions/3.5/lib/python3.5', '/usr/local/Cellar/python3/3.5.1/Frameworks/Python.framework/Versions/3.5/lib/python3.5/plat-darwin', '/Users/erics/vpy35/lib/python3.5/site-packages']

Note that the multiprocessor one uses the source directory, and the non-multiproc one uses the installed directory (in the second entry).

coverage run --append --include=PIL/* selftest.py
coverage run --append --include=PIL/* -m nose -vx Tests/test_*.py
python selftest.py
python setup.py test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to do coverage without having it in the defaults?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(previous comment deleted)

Updated to not use python setup.py test, and therefore not use setup.cfg's defaults.

@wiredfool
Copy link
Member

#2911 fixes the path issue, I think once and for all.

@wiredfool
Copy link
Member

Merged with #2911

@wiredfool wiredfool closed this Dec 29, 2017
@hugovk
Copy link
Member Author

hugovk commented Dec 30, 2017

Thanks for finishing this up!

@hugovk hugovk deleted the pytest branch December 30, 2017 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants