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

🧪 TESTS: convert AiidaTestCase -> pytest #4782

Closed
wants to merge 1 commit into from

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Feb 25, 2021

Conversion process:

  • unittest2pytest -n -w path/to/file (see https://github.com/pytest-dev/unittest2pytest)
  • add # pylint: disable=no-self-use
  • replace AiidaTestCase with @pytest.mark.usefixtures('aiida_profile_clean_class') (I would say this is not recommended unless there are particular performance reasons, since every test should be independent), @pytest.mark.usefixtures('aiida_profile_clean'), or using an autouse fixture (see below)
    • replace def setUp(self): with def setup_method(self): or an autouse fixture
  • remove from aiida.backends.testbase import AiidaTestCase
  • change @unittest.skip -> @pytest.mark.skip, @unittest.skipIf -> @pytest.mark.skipif

The were some issues whereassertAlmostEquals had been used for strange cases, related to this I also added an __abs__ method to NumericType

Annoyingly there are some limitations of using classes with pytest, one being that a class usefixtures decorator is called AFTER a setup_class method (see
https://stackoverflow.com/questions/31484419/pytest-setup-class-after-fixture-initialization_) so any nodes created in setup_class will be wiped from the database by aiida_profile_clean_class.
Also class methods do not accept fixtures.
This makes it difficult to do an exact 1-to-1 replacement of any setUpClass -> setup_class methods used, and also replace the use of self.computer

edit: https://docs.pytest.org/en/latest/how-to/unittest.html#using-autouse-fixtures-and-accessing-other-fixtures allows for using fixtures per test, and so can solve the use of self.computer etc e.g.

# pylint: disable=attribute-defined-outside-init,no-self-use,unused-argument

class MyTestClass:
    @pytest.fixture(autouse=True)
    def init_db(self, aiida_profile_clean, backend, aiida_localhost):
        self.backend = backend
        self.computer = aiida_localhost
    def test_something(self):
        self.computer

or if you don't need any instance attributes:

@pytest.mark.usefixtures('aiida_profile_clean')
class MyTestClass:
   def test_something(self):

Process:

- unittest2pytest -n -w path/to/file
- add `# pylint: disable=no-self-use`
- replace `AiidaTestCase` with `@pytest.mark.usefixtures('clear_database_before_test_class')`
- remove from `aiida.backends.testbase import AiidaTestCase`
- change @unittest.skip -> @pytest.mark.skip, @unittest.skipIf -> @pytest.mark.skipif

The were some issues where`assertAlmostEquals` had been used for strange cases

Annoyingly there are some limitations, one being that the class fixture is called AFTER a `setup_class` method (see
https://stackoverflow.com/questions/31484419/pytest-setup-class-after-fixture-initialization_)
and also class methods don't accept fixtures. This makes it difficult to do a 1-to-1 replacement of any `setUpClass` methods used, and also replace the use of `self.computer`
@chrisjsewell
Copy link
Member Author

cc @ltalirz, this should pass all tests for the files I have changed (plenty more to do), so if you want to have a look do a quick "pre-review" for any issues, before I apply this to any more files?

@chrisjsewell chrisjsewell changed the title 🧪 TESTS: AiidaTestCase -> pytest 🧪 TESTS: convert AiidaTestCase -> pytest Feb 25, 2021
@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #4782 (3361712) into develop (c07e3ef) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4782      +/-   ##
===========================================
- Coverage    79.56%   79.52%   -0.04%     
===========================================
  Files          515      515              
  Lines        36812    36817       +5     
===========================================
- Hits         29285    29274      -11     
- Misses        7527     7543      +16     
Flag Coverage Δ
django 74.19% <100.00%> (-0.07%) ⬇️
sqlalchemy 73.13% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/manage/tests/pytest_fixtures.py 93.34% <100.00%> (+0.28%) ⬆️
aiida/orm/nodes/data/numeric.py 91.81% <100.00%> (+1.98%) ⬆️
aiida/backends/general/abstractqueries.py 59.68% <0.00%> (-4.03%) ⬇️
aiida/orm/implementation/django/groups.py 82.86% <0.00%> (-2.14%) ⬇️
aiida/orm/groups.py 91.31% <0.00%> (-1.44%) ⬇️
aiida/orm/implementation/sqlalchemy/groups.py 86.09% <0.00%> (-1.03%) ⬇️
...iida/orm/implementation/sqlalchemy/querybuilder.py 81.15% <0.00%> (-0.57%) ⬇️
aiida/orm/querybuilder.py 79.64% <0.00%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c07e3ef...3361712. Read the comment docs.

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 @chrisjsewell - looks good to me, no obvious issues come to mind.

when transforming more tests you may run into the fact that the AiidaTestCase is a bit more rigorous in cleaning not only the db but also resetting various caches (see the clean_db method).
These may need to be ported over to the reset_db method of the ProfileManager

@ltalirz
Copy link
Member

ltalirz commented Feb 22, 2022

I guess this can then also be closed?

@chrisjsewell
Copy link
Member Author

I guess this can then also be closed?

Yep 👍, superseded by #5372

Perhaps some of the tips for writing tests, in the initial comment, should be copied somewhere?

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.

2 participants