reorganize plugin testing code #3319

ltalirz · 2019-09-12T15:49:19Z

The "helper" code for plugin testing is reorganized from a single file fixtures.py (that actually did not contain a single fixture) over multiple files.

manage/tests/__init__.py for the manager, which is now called TestManager
(it has nothing to do with fixtures)
unittest_classes.py for test classes & runners for use with unittest
pytest_fixtures.py for the fixtures for use with pytest (=the main new content in this PR)

In order to avoid breaking the API, the old way of importing the main classes is still supported (with a corresponding deprecation warning).

Further additions:

TestManager can now use either a TemporaryProfileManager (which will set up the postgres cluster etc.) or a ProfileManager (which will use an existing test Profile). The ProfileManager is used automatically when the TEST_AIIDA_PROFILE environment variable is set.
Fixes an issue where the temporary postgres cluster could remain running when a test failed

Up for discussion:

The file names unittest_classes.py and pytest_fixtures.py are a bit long.
I previously tried unittest.py and pytest.py but this caused confusion with the external packages of the same name. Open to alternative suggestions
One of the new fixtures (aiida_localhost_code) is of a different type than the others, since it needs extra parameters. Depending on the needs of a particular code/plugin even the two provided may not be enough.
I still think it is useful to have - if people feel this should not be in aiida-core, I can also remove it again.

plugin testing code is split up from a single file fixtures.py (that actually did not contain a single fixture) over multiple files * one for the manager, which is now called TestManager (it has nothing to do with fixtures) * one for test classes & runners for use with unittest * one for (newly added) fixtures for use with pytest

ltalirz · 2019-09-12T18:19:34Z

Sorry for the follow-up commits. Tests pass on my fork now, and I'm not planning any further commits. Let me know what you think!

yakutovicha · 2019-09-16T10:38:06Z

thanks, @ltalirz, I am going to adapt the cp2k plugin to use the fixtures introduced here. Once they work fine I am going to approve your PR.

yakutovicha · 2019-09-16T14:22:02Z

We should add a possibility to not remove the folder where the calculation was run, useful for the tests.

both unittest test classes and pytest fixtures will now use the backend defined in the `TEST_AIIDA_BACKEND` environment variable.

for consistency (used nowhere else in the code). + see https://stackoverflow.com/a/6930223

by specifying the environment variable TEST_AIIDA_PROFILE=profilename one can use an existing profile instead of setting up a test profile automatically.

yakutovicha · 2019-09-17T04:34:29Z

thanks, @ltalirz, for the ebd3f68. I think this feature is absolutely critical to make tests useful for the developers.

yakutovicha · 2019-09-17T04:43:13Z

after pulling the last changes I get the following error while running pytest for the aiida-cp2k plugin:

self = <aiida.manage.configuration.profile.Profile object at 0x11ea78a10>

    @property
    def default_user(self):
>       return self._attributes[self.KEY_DEFAULT_USER]
E       KeyError: 'default_user_email'

../aiida_core/aiida/manage/configuration/profile.py:124: KeyError

ltalirz · 2019-09-17T07:09:47Z

Yes, adding the possibility to avoid the profile creation required a lot of reorganization and I still need to fix a few things. Will let you know.

…

On Tue, Sep 17, 2019 at 6:43 AM Aliaksandr Yakutovich < ***@***.***> wrote: after taking the last changes I get the following error while running pytest for the aiida-cp2k plugin: self = <aiida.manage.configuration.profile.Profile object at 0x11ea78a10> @Property def default_user(self): > return self._attributes[self.KEY_DEFAULT_USER] E KeyError: 'default_user_email' ../aiida_core/aiida/manage/configuration/profile.py:124: KeyError — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3319?email_source=notifications&email_token=AAIBEPODNTHBS5ZFQZABPUTQKBN6HA5CNFSM4IWGMDR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD63IQTQ#issuecomment-532056142>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIBEPM5TW6WDDZPAIMNGWDQKBN6HANCNFSM4IWGMDRQ> .

sphuber · 2019-09-17T16:31:20Z

To give an idea of how testing is done in aiida-quantumespresso.

Fixtures for pytest are written in this conftest.py file. Some that are added in this PR directly overlap so they could be removed and use directly from aiida-core.
There is a fixture to test CalcJob classes. Example usage can be found here for PwCalculation.
Similar fixture exists to test Parser implementations. It essentially mocks a completed CalcJobNode with the retrieved folder data node that contains the retrieved files. Example usage for PwParser shows.

The automatic comparison files generation and checking is done through the pytest-regressions plugin which is added as a dependency. With this pytest plugin and the two fixtures, it is really easy to write and run tests for calculation jobs and parsers without actually having to run the calculations. Note of course, as mentioned before, that some tests that actually run the code will still be useful and desirable.

aiida/manage/tests/pytest_fixtures.py

ltalirz · 2019-09-18T07:55:49Z

Just to have everything in one place - below the pointers from @greschd concerning aiida-pytest:

example for the config file: https://github.com/greschd/aiida-tbextraction/blob/master/tests/config_example.yml
On the actual machine I would rename this to ‘config.yml’ (which is in .gitignore), and make the necessary changes.
Getting the config: https://github.com/greschd/aiida_pytest/blob/migrate_v1/aiida_pytest/_configure.py
Setup of computers: https://github.com/greschd/aiida_pytest/blob/migrate_v1/aiida_pytest/_computer.py
Setup of codes: https://github.com/greschd/aiida_pytest/blob/migrate_v1/aiida_pytest/_code.py

One thing that I’ve found to be helpful is having an option to wait for user input before wiping the test DB. That allows for inspecting the state better before it’s gone. See here: https://github.com/greschd/aiida_pytest/blob/migrate_v1/aiida_pytest/_configure.py#L152

sphuber · 2019-10-18T13:53:50Z

@ltalirz was this something that needed to go in before the release?

ltalirz · 2019-10-18T14:23:04Z

I'll continue to work on this today; also to add at least the possibility to use the pytest fixtures together with an existing AiiDA profile.

However, this is not absolutely necessary for the 1.0 release.

ltalirz · 2019-10-18T17:59:32Z

closing this while doing some more work.

ltalirz · 2019-10-18T18:09:14Z

@sphuber Do you remember whether configuring logging is needed here:
https://github.com/aiidateam/aiida-quantumespresso/blob/22d54c868c3c2322a83116476ee369c07bede800/tests/conftest.py#L31-L32

I.e. do you think this is something the fixture manager should do automatically?

ltalirz · 2019-10-18T18:21:22Z

@sphuber
Regarding the generate_calc_job and generate_calc_job_node fixtures: The generate_calc_job fixture seems compact and agnostic and could be added as-is - the generate_calc_job_node fixture less so.
If you would like to include the latter here as well, would you mind making a version that is entirely aiida-qe agnostic?

Finally, as far as I can tell, the fixtures don't make direct use of pytest-regressions (but it's rather in how they are used).
What would be the best way to make it easy for people to use it? Should we just include something in the documentation?

P.S. You have rights to push to my fork, so feel free to proceed however is easiest for you.

* fix name according to suggestion * fix bug in database query since the code label no longer referred to the full label (including hte computer), `Code.objects.get(label='network@computer')` would always turn up empty-handed, causing the utility to create a new code. Now, we're searching all available codes with the `network` label and simply take the first one (if it exists).

sphuber · 2019-10-18T19:05:37Z

@sphuber Do you remember whether configuring logging is needed here:
https://github.com/aiidateam/aiida-quantumespresso/blob/22d54c868c3c2322a83116476ee369c07bede800/tests/conftest.py#L31-L32

I.e. do you think this is something the fixture manager should do automatically?

I actually happen to have added that just today. It was necessary to get the DbLogHandler loaded in time. Otherwise the logging in Parser classes, that I trigger through the parser_from_node function would not end up triggering the handler, therefore attaching the log messages to the node. The weird thing was that the first test the handlers were not configured, but at the second time it was. So it seems the normal logging configuration pathway was being triggered to late. I didn't have time to figure out why, so put this in for the time being.

ResourceWarning introduced in python 3.2

ltalirz · 2019-10-24T14:41:39Z

@sphuber This would basically be ready for review.

I haven't yet incorporated Dominik's additions - perhaps I'll find time but if not I think this could also be done in a separate PR.

With `autouse=True`, there is no straightforward way for plugin authors to disable use of a fixture. While this makes sense for the `aiida_profile` fixture, we've encountered cases where resetting the database in between tests may lead to issues. This problem is solved by keeping the fixture available but requiring users to depend on it explicitly.

sphuber

Thanks @ltalirz , some questions and comments

aiida/manage/tests/pytest_fixtures.py

aiida/manage/tests/unittest_classes.py

docs/source/developer_guide/plugins/plugin_tests.rst

sphuber · 2019-10-25T07:43:10Z

aiida/orm/entities.py

+        Resets collection instance back to its initial state (to be constructed from scratch when next requested).
+        Useful to clear any internal caches of the collection.
+        """
+        cls._COLLECTIONS = datastructures.LazyStore()


I don't think this does what you intend it to do. This will reset the collection for all entities and backend. So when you call User.object.reset it will reset all collections not just that of User. I would really go for the alternative you proposed for now and implement a specific default user reset on the User collection

Thanks, for the moment I've implemented the modification described in #3462

I can still change it if this is thought to be the worse solution.

yakutovicha · 2019-10-25T09:04:32Z

Just a comment from my side: I used those modifications to test aiida-cp2k plugin and it worked fine on my local machine: aiidateam/aiida-cp2k#49

Beware that on travis the tests still fail because they are based on 1.0.0b6 release of aiida_core.

Co-Authored-By: Sebastiaan Huber <[email protected]>

ltalirz · 2019-10-25T12:49:33Z

@sphuber Thanks a lot for the thorough review and the great suggestions.
I've gone through all your comments now & pushed some fixes.

* rename TEST_AIIDA_PROFILE => AIIDA_TEST_PROFILE * rename TEST_AIIDA_BACKEND => AIIDA_TEST_BACKEND * add warning when setting AIIDA_TEST_PROFILE while running `verdi devel tests` * make test profile configurable at the test_manager level, so that the context manager can be used without having to mess with environment variables.

ltalirz · 2019-10-25T17:04:42Z

@sphuber I've made the changes we discussed.
I still left three comments open - feel free to resolve or to follow up further.

aiida/orm/entities.py

…into add-pytest-fixtures2

implement specific cache reset method for User.objects

ltalirz · 2019-10-28T07:52:13Z

@sphuber 61d696e includes the modification we discussed (moving the cache reset to the User collection only).

If there's anything else left, let me know!

sphuber · 2019-10-28T13:15:13Z

Thanks a lot @ltalirz

greschd · 2019-10-28T16:03:23Z

@ltalirz I'm just migrating aiida-pytest to match these changes -- is there a nicer way to get at the root directory than through the aiida_profile fixture's aiida_profile._manager.root_dir?

ltalirz · 2019-10-28T16:28:30Z

@greschd probably not - if you have any small suggestions for improvements, now would be a good time to make a PR.

By the way, sorry for not yet having included more from aiida-pytest's features, it was just lack of time on my side. I'll open an issue so that these things don't get lost and we can keep improving the fixtures.

greschd · 2019-10-28T17:00:21Z

I haven't had the time to look at it in detail, but from the surface it looks pretty good - better than that hot mess that is aiida-pytest for sure 😅

ltalirz requested review from yakutovicha and sphuber September 12, 2019 15:49

add simple test for pytest fixtures

c1b3f5a

ltalirz force-pushed the add-pytest-fixtures2 branch from 957b12d to c1b3f5a Compare September 12, 2019 16:42

update docs

5b1967c

ltalirz added 3 commits September 16, 2019 17:39

add automatic backend handling

84977bf

both unittest test classes and pytest fixtures will now use the backend defined in the `TEST_AIIDA_BACKEND` environment variable.

remove double underscore class variables

ca8f1c6

for consistency (used nowhere else in the code). + see https://stackoverflow.com/a/6930223

make temporary test profile optional

ebd3f68

by specifying the environment variable TEST_AIIDA_PROFILE=profilename one can use an existing profile instead of setting up a test profile automatically.

fixes for test_test_manager

57d452a

greschd reviewed Sep 17, 2019

View reviewed changes

aiida/manage/tests/pytest_fixtures.py Outdated Show resolved Hide resolved

ltalirz closed this Oct 18, 2019

ltalirz added 3 commits October 18, 2019 20:59

Merge branch 'develop' into add-pytest-fixtures2

8c0b537

fix linter and docs

d37cfbe

fix ResourceWarning issue

4f80c13

ResourceWarning introduced in python 3.2

ltalirz force-pushed the add-pytest-fixtures2 branch from 4879d3c to 3b5e9ea Compare October 24, 2019 20:08

sphuber requested changes Oct 25, 2019

View reviewed changes

ltalirz and others added 2 commits October 25, 2019 14:42

Apply suggestions from code review

a6f558c

Co-Authored-By: Sebastiaan Huber <[email protected]>

implement suggestions by @sphuber

a9568f3

ltalirz added 2 commits October 25, 2019 19:02

Merge branch 'develop' into add-pytest-fixtures2

43fea6c

Merge branch 'develop' into add-pytest-fixtures2

403fd63

sphuber reviewed Oct 25, 2019

View reviewed changes

aiida/orm/entities.py Outdated Show resolved Hide resolved

ltalirz added 3 commits October 27, 2019 23:41

Merge branch 'add-pytest-fixtures2' of github.com:ltalirz/aiida_core …

1a636d0

…into add-pytest-fixtures2

change cache reset to work on User.obects only

61d696e

implement specific cache reset method for User.objects

Merge branch 'develop' into add-pytest-fixtures2

2999853

ltalirz force-pushed the add-pytest-fixtures2 branch from fb1bebc to 4ae8ade Compare October 28, 2019 08:41

rename TEST_AIIDA_BACKEND on travis

70c234a

ltalirz force-pushed the add-pytest-fixtures2 branch from 4ae8ade to 70c234a Compare October 28, 2019 08:43

ltalirz and others added 2 commits October 28, 2019 10:40

add test for aiida_local_code_factory

912c3c2

Merge branch 'develop' into add-pytest-fixtures2

a88e4a9

sphuber approved these changes Oct 28, 2019

View reviewed changes

sphuber merged commit e85e534 into aiidateam:develop Oct 28, 2019

ltalirz mentioned this pull request Oct 28, 2019

verdi code setup does not check the remote absolute path #868

Closed

chrisjsewell mentioned this pull request Nov 22, 2022

👌 IMPROVE failure logging aiidateam/aiida-test-cache#63

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reorganize plugin testing code #3319

reorganize plugin testing code #3319

ltalirz commented Sep 12, 2019 •

edited

Loading

ltalirz commented Sep 12, 2019

yakutovicha commented Sep 16, 2019

yakutovicha commented Sep 16, 2019

yakutovicha commented Sep 17, 2019

yakutovicha commented Sep 17, 2019 •

edited

Loading

ltalirz commented Sep 17, 2019 via email

sphuber commented Sep 17, 2019

ltalirz commented Sep 18, 2019

sphuber commented Oct 18, 2019

ltalirz commented Oct 18, 2019

ltalirz commented Oct 18, 2019

ltalirz commented Oct 18, 2019

ltalirz commented Oct 18, 2019 •

edited

Loading

sphuber commented Oct 18, 2019

ltalirz commented Oct 24, 2019

sphuber left a comment

sphuber Oct 25, 2019

ltalirz Oct 25, 2019

yakutovicha commented Oct 25, 2019

ltalirz commented Oct 25, 2019

ltalirz commented Oct 25, 2019

ltalirz commented Oct 28, 2019

sphuber commented Oct 28, 2019

greschd commented Oct 28, 2019

ltalirz commented Oct 28, 2019

greschd commented Oct 28, 2019

reorganize plugin testing code #3319

reorganize plugin testing code #3319

Conversation

ltalirz commented Sep 12, 2019 • edited Loading

ltalirz commented Sep 12, 2019

yakutovicha commented Sep 16, 2019

yakutovicha commented Sep 16, 2019

yakutovicha commented Sep 17, 2019

yakutovicha commented Sep 17, 2019 • edited Loading

ltalirz commented Sep 17, 2019 via email

sphuber commented Sep 17, 2019

ltalirz commented Sep 18, 2019

sphuber commented Oct 18, 2019

ltalirz commented Oct 18, 2019

ltalirz commented Oct 18, 2019

ltalirz commented Oct 18, 2019

ltalirz commented Oct 18, 2019 • edited Loading

sphuber commented Oct 18, 2019

ltalirz commented Oct 24, 2019

sphuber left a comment

Choose a reason for hiding this comment

sphuber Oct 25, 2019

Choose a reason for hiding this comment

ltalirz Oct 25, 2019

Choose a reason for hiding this comment

yakutovicha commented Oct 25, 2019

ltalirz commented Oct 25, 2019

ltalirz commented Oct 25, 2019

ltalirz commented Oct 28, 2019

sphuber commented Oct 28, 2019

greschd commented Oct 28, 2019

ltalirz commented Oct 28, 2019

greschd commented Oct 28, 2019

ltalirz commented Sep 12, 2019 •

edited

Loading

yakutovicha commented Sep 17, 2019 •

edited

Loading

ltalirz commented Oct 18, 2019 •

edited

Loading