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 unit tests from aiida to top-level tests #3715

Merged
merged 2 commits into from
Jan 16, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jan 16, 2020

Fixes #3659

I have split the work in two commits to make inspecting the changes as easy as possible.

  • Move the files from aiida to tests. Besides adapting the .pre-commit-config.yaml this commit does not contain anything else and should not have to be inspected.
  • Adapt all imports in moved files to make things work again. This is the commit that should be looked at a bit.

Note that the solution of relative imports that I used is an ugly one I find. Most of the existing utilities can be cleaned up and be turned into pytest fixtures. However, I want to keep this for a separate PR.

@sphuber sphuber requested a review from ltalirz January 16, 2020 11:02
@ltalirz ltalirz self-requested a review January 16, 2020 12:33
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @sphuber for making progress with this!

If you look at the travis logs, you'll notice that it is actually not yet running the tests in tests/; you'll need to adapt the pytests.ini.

Also, there are about a hundred tests left in aiida
https://github.com/aiidateam/aiida-core/pull/3715/checks?check_run_id=392973225
Do you want to move them in a separate PR?

@sphuber
Copy link
Contributor Author

sphuber commented Jan 16, 2020

thanks @sphuber for making progress with this!

Thanks for the review

If you look at the travis logs, you'll notice that it is actually not yet running the tests in tests/; you'll need to adapt the pytests.ini.

Yup, forgot to change this. I will also add an explicit path in .github/workflows/tests.sh

Also, there are about a hundred tests left in aiida
https://github.com/aiidateam/aiida-core/pull/3715/checks?check_run_id=392973225
Do you want to move them in a separate PR?

No, I forgot about those and will add them here.

This will prevent them from being included in the distribution that is
uploaded to PyPI making the download a lot quicker.

Note that this commit only moves the files without touching them. This
means that the tests won't actually run since, for example, various
imports are broken. For clarity these changes will be done in a
following commit to not drown out the important changes between files
that were just moved around.

Note that the `django` and `sqlachemy` modules in `tests/backend`, which
contain backend specific tests, are prefixed with `aiida_`, otherwise
imports using either `from django` or `from sqlalchemy` will target
incorrectly those modules, instead of the actual libraries.
The unit tests that were moved in the previous commit often imported
utilities from a module that used to be located in the main package
`aiida.backends.tests.utils`. This module has also been moved to the
`tests` top level module.

These utilities should really in most cases become pytest fixtures but
that is left for a later time.
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Thanks @sphuber - I've looked through all changed files (wasn't too long after all) and it looks fine.
Also checked that number of tests on django & sqla (python 3.7) are the same as before.

Looks fine to me!
I suggest you change the title since you move all unit tests, not just the ones in backends.

All that remains is a minor update here: https://github.com/aiidateam/aiida-core/wiki/Writing-tests#running-existing-tests

@sphuber sphuber changed the title Move unit tests from aiida.backends.tests to top-level tests Move unit tests from aiida to top-level tests Jan 16, 2020
@sphuber sphuber merged commit 6ce6cd4 into aiidateam:develop Jan 16, 2020
@sphuber sphuber deleted the fix_3659_move_tests branch January 16, 2020 15:43
@sphuber
Copy link
Contributor Author

sphuber commented Jan 16, 2020

Looks fine to me!
I suggest you change the title since you move all unit tests, not just the ones in backends.

Done

All that remains is a minor update here: https://github.com/aiidateam/aiida-core/wiki/Writing-tests#running-existing-tests

I will see if I find sometime to adapt it. There is quite some stuff there that was already outdated even unrelated to this PR.

@ltalirz
Copy link
Member

ltalirz commented Jan 16, 2020

I've adapted the wiki now to the changed test location.
We will do another update at some point for pytest suggestions.

One thing I noticed now is that you changed the folder structure for the transport tests, i.e. the plugin-specific tests are no longer in a subfolder plugins.
I guess the test discovery did not rely on this (?)

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.

pytest follow-up
2 participants