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

Distribute tests #669

Merged
merged 10 commits into from
Oct 24, 2016
Merged

Distribute tests #669

merged 10 commits into from
Oct 24, 2016

Conversation

kohr-h
Copy link
Member

@kohr-h kohr-h commented Oct 21, 2016

Unfortunately I realized too late that the tests are not included, neither in the pip nor the conda packages. The problem is basically that odl/test is not a package and thus not included. We actually explicitly excluded anything containing "test" in setup.py, ahem.

Anyway, it turns out that the python setup.py sdist and bdist_wheel commands can rather easily be convinced to include extra stuff using the package_data parameter AND recursively adding odl/test in MANIFEST.in. However, for python setup.py install, you also have to give the include_package_data=True option (of course, why would I want to install with data when I explicitly give package_data???).

Next issue: conftest.py is not installed, and without it, the extra plugins (--largescale and such) don't work. No test runs since the options in the corresponding files are not recognized. Putting conftest.py one level below didn't work either for a reason I don't remember. Here the solution was to go for the second item here and follow the instructions here to make an entry point for pytest -- which now resides in odl.util.pytest_plugins.py. Here the description was actually very good, and the approach worked immediately.

So now that this is done, I think we need a new release 0.5.1 (sorry about that). I'll put the correct tag into conda/meta.yaml and merge when we agree on the changes.

@kohr-h
Copy link
Member Author

kohr-h commented Oct 21, 2016

Perhaps it's not too bad that Travis CI was down due to connection problems at GitHub (hard to say whose fault), so the new installation doc is not yet deployed. Seems fixed now. No, still problems. Tests fail during apt-get update due to connection timeouts. What the f?

@kohr-h
Copy link
Member Author

kohr-h commented Oct 21, 2016

See Travis CI status

@kohr-h
Copy link
Member Author

kohr-h commented Oct 21, 2016

Another remark: Sudden pytest warnings are not my fault:

pytest-dev/pytest#2005

@adler-j
Copy link
Member

adler-j commented Oct 22, 2016

We need to look into how numby solves this, in the end this really is something we need.

@adler-j
Copy link
Member

adler-j commented Oct 22, 2016

Above comment was me thinking this was a issue ^^.

Anyway nice pull, but does removing conftest.py not screw up running pytest, or does it find it somehow?

package_dir={'odl': 'odl'},
package_data={'odl': find_tests()},
include_package_data=True,
entry_points={'pytest11': ['odl_plugins = odl.util.pytest_plugins']},
Copy link
Member

Choose a reason for hiding this comment

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

Does this make pytest find it? What does 11 mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is one of the 4 or 5 ways to extend pytest by custom "plugins". The mechanism I used is described here. The name "pytest11" is fixed and magic :-)

@adler-j
Copy link
Member

adler-j commented Oct 22, 2016

How about the pytest.ini file?

@@ -467,5 +467,5 @@ If that does not help, `make an issue on GitHub <https://github.com/odlgroup/odl
.. _CVXPY: http://www.cvxpy.org/en/latest/
.. _odlcuda: https://github.com/odlgroup/odlcuda
.. _CUDA toolkit: https://developer.nvidia.com/cuda-toolkit
.. _ASTRA tomography toolbox: https://github.com/astra-toolbox/astra-toolbox
.. _ASTRA: https://github.com/astra-toolbox/astra-toolbox
Copy link
Member

Choose a reason for hiding this comment

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

This link has to be wrong now?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the other one is inline now, I don't see a point in 3 different targets linking to the same page.

for filename in filenames:
basename, suffix = os.path.splitext(filename)
if (suffix == '.py' and
(basename.startswith('test') or
Copy link
Member

Choose a reason for hiding this comment

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

It is actually supposed to be _test and test_

See:

http://doc.pytest.org/en/latest/goodpractices.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I had that first, it was correct. Dunno why I changed it.

@@ -61,6 +56,23 @@ def run_tests(self):
errno = pytest.main(self.pytest_args)
sys.exit(errno)


test_path = os.path.join(root_path, 'odl/test')
Copy link
Member

Choose a reason for hiding this comment

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

os.path.join(root_path, 'odl', test')

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, of course.

@adler-j
Copy link
Member

adler-j commented Oct 22, 2016

Looks good except for the minor comments. Are we sure this works? If we we need to make a 5.1 release, perhaps you can look into pypi test (https://wiki.python.org/moin/TestPyPI) before the actual release.

@kohr-h
Copy link
Member Author

kohr-h commented Oct 22, 2016

How about the pytest.ini file?

It can be included as extra package file, however only under odl, not at top repo level (in this case one can specify the file in MANIFEST.in, but it won't be put into the final package since it's counted to metadata).
The price we pay is either a duplicate at top level or that we need to invoke pytest odl in the future, which is also okay IMO.

@kohr-h
Copy link
Member Author

kohr-h commented Oct 22, 2016

Everything is fixed, and I tested all combinations of Python 2/3 and source/wheel/conda packages. Works smoothly.
You can try the packages yourself with

pip install -i https://testpypi.python.org/pypi odl

Good suggestion with the test server.

@kohr-h
Copy link
Member Author

kohr-h commented Oct 24, 2016

Bumpety

@adler-j
Copy link
Member

adler-j commented Oct 24, 2016

Fine by me!

@kohr-h kohr-h merged commit 9054a0f into master Oct 24, 2016
@kohr-h kohr-h deleted the distribute_tests branch October 24, 2016 14:19
@adler-j
Copy link
Member

adler-j commented Oct 24, 2016

5.1 release comming?

@kohr-h
Copy link
Member Author

kohr-h commented Oct 24, 2016

Done!

@adler-j
Copy link
Member

adler-j commented Oct 24, 2016

Trying install in a VM

@adler-j
Copy link
Member

adler-j commented Oct 24, 2016

Worked perfectly from pip install at least! Roughly 3300 tests, no failures etc. Good work!

@kohr-h
Copy link
Member Author

kohr-h commented Oct 24, 2016

Phew! :-)

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.

2 participants