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

Move tests into tests directory #1698

Merged
merged 30 commits into from
May 19, 2021
Merged

Move tests into tests directory #1698

merged 30 commits into from
May 19, 2021

Conversation

graeme-winter
Copy link
Contributor

Kept the structure the same as far as possible. Had to fix references in flight. I think all the tests should pass, will run regression tests in a mo.

Motivation: consistency

@Anthchirp
Copy link
Member

If you move the tests into /tests with the aim of eventually removing them from the python package, then __init__.pys should not be moved along, which will also prevent the files from becoming importable. If the tests have import dependencies between them then they need to be rewritten/moved back later, and then it'd be easier to just leave them where they are.

@graeme-winter
Copy link
Contributor Author

If you move the tests into /tests with the aim of eventually removing them from the python package, then __init__.pys should not be moved along, which will also prevent the files from becoming importable. If the tests have import dependencies between them then they need to be rewritten/moved back later, and then it'd be easier to just leave them where they are.

My motivation is wanting to be able to find the tests easily without needing to use the UNIX find command - future Python package was motivation for renaming test->tests but that is "while I was there"

@graeme-winter
Copy link
Contributor Author

Hmmm would seem there is a lot of weird crosstalk between tests (i.e. there are tests which are very not self-contained and imported bits of other tests from other folders e.g.

――――――― ERROR collecting tests/command_line/test_search_beam_position.py ―――――――
ImportError while importing test module '/Users/graeme/git/dials/modules/dials/tests/command_line/test_search_beam_position.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../conda_base/python.app/Contents/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/command_line/test_search_beam_position.py:11: in <module>
    from dials.algorithms.indexing.test_index import run_indexing
E   ModuleNotFoundError: No module named 'dials.algorithms.indexing.test_index'

which weirdly does not cause test failure)

@Anthchirp
Copy link
Member

Collection errors are errors, which are a separate category from test failures. Should still lead to failed builds on Azure

Yes, I know I said we don't need those. But turns out that pytest really
wants to be able to distinguish files, and if there are no __init__.py
files at all then files are imported individually, and therefore you
can't have two test files with the same name.

This also means you only really need to add an __init__.py file if you
get one of those

import file mismatch:
imported module 'test_XXX' has this __file__ attribute:
  ...
which is not the same as the test file we want to collect:
  ...
HINT: remove __pycache__ / .pyc files and/or use a unique basename for
your test file modules

errors.
@Anthchirp
Copy link
Member

minimal command to reproduce the issue:

pytest doc/examples/test_boilerplate.py::test_boilerplate tests/algorithms/indexing/basis_vector_search/test_combinations.py::test_filter_known_symmetry_no_matches --regression

This can throw off all sorts of things in other tests
Merge remote-tracking branch 'origin/main' into tests-move-directory
@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #1698 (d6cc25a) into main (f0d524a) will decrease coverage by 0.00%.
The diff coverage is 97.43%.

@@            Coverage Diff             @@
##             main    #1698      +/-   ##
==========================================
- Coverage   67.13%   67.13%   -0.01%     
==========================================
  Files         615      615              
  Lines       69053    69051       -2     
  Branches     9622     9622              
==========================================
- Hits        46356    46354       -2     
+ Misses      20765    20764       -1     
- Partials     1932     1933       +1     

@Anthchirp
Copy link
Member

xfel_regression refers to from dials.test.(...) import ... in tst_ensemble_refinement.py, so that is the likely reason for the x_r failure.

But this PR is ready for merging.

@Anthchirp Anthchirp merged commit 1ae2f58 into main May 19, 2021
@Anthchirp Anthchirp deleted the tests-move-directory branch May 19, 2021 15:09
@phyy-nx
Copy link
Member

phyy-nx commented May 19, 2021

I've patched xfel_regression to refer to the new location in tst_ensemble_refinement.py.

@Anthchirp
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants