pytest fixtures vs. "old" unittests crash on CI

[CasperWA]

The pytest fixtures work fine when testing pytest-style tests locally, but not when run on GitHub Actions.

This issue has been mentioned in #3738, specifically in this comment, as well as in #3974, where the issue can be directly seen in the CI runs here and a previous run here.

[ltalirz]

@CasperWA can this be closed?
I don't recall any recent issues with running pytest on CI

[greschd]

Just a general FYI, sometimes this can be due to interference from other (old-style) tests. It's happened to me that running the single relevant test (locally) works fine but once the whole test suite (on CI) is run it will fail.
The temporary workaround is to use the clear_database_before_test fixture; eventually (once all old-style tests are gone) we can remove the fixtures.

Can't see the logs for these CI runs now, but I think the error message for that particular problem is usually something about no User being defined.

[ltalirz]

Ok, then it's fine to leave this open as a reminder

[chrisjsewell]

The temporary workaround is to use the clear_database_before_test fixture; eventually (once all old-style tests are gone) we can remove the fixtures.

@greschd why do you think clear_database_before_test is a workaround?
To my knowledge the database does not get cleared automatically between tests and, for reproducible tests (i.e. that cannot be impacted by earlier ones in the same run), you always want to clear the database (if the test does indeed requires its use).

The only reason to "opt-out" of this IMO is when you have a set of tests, in a single class/module, where you know there will be no interference, so you only need to apply it once for efficiency.

On a related note, I also think that clear_database_after_test should just be removed, because IMO it never makes sense to use this.

[greschd]

@greschd why do you think clear_database_before_test is a workaround?

Most tests should work on any "consistent" database state, i.e. one where a profile is properly configured. Those shouldn't need clear_database_before_test. It's a matter of perspective / approach, either each test is responsible for creating a workable DB (from scratch), or each test is responsible for leaving behind a consistent DB state.
That latter approach is less clean - as evidenced by the ability to "poison" other tests, but can be more efficient.

In plugin tests we sometimes even share part of the database setup (codes, computers) among tests (and do it only once), so there clear_database_before_tests won't work. At least, not without some way of preserving part of the DB state / reverting back to as it was at the start of the test.

While I don't remember using clear_database_after_test myself, that was actually the first one to be implemented (which is why clear_database is an alias for that); so I'm guessing there's a use case.

[greschd]

Ok, then it's fine to leave this open as a reminder

We can also close it if it's not an active issue; IMO the difference in searchability between open and closed issues isn't that significant.

[chrisjsewell]

Most tests should work on any "consistent" database state

I think thats a dangerous assumption, but yes I agree that when you are sure this is not the case, and/or you need a certain DB state for a set of tests, then you don't need to use clear_database_before_tests

I really don't think there should be a use case for clear_database_after_test; it just leads to confusion over which one to use.

[greschd]

I really don't think there should be a use case for clear_database_after_test; it just leads to confusion over which one to use.

Well, if you know that you're trashing the DB (messing with the profile / User or some-such) it could be useful. I guess we'd have to go looking into the plugin test codes to see if anyone actually uses it.

[chrisjsewell]

Sure, its not a massive issue obviously, I just think that test isolation should always be the main priority, with performance considerations secondary.

[ltalirz]

I guess we'd have to go looking into the plugin test codes to see if anyone actually uses it.

Not on github at least: https://github.com/search?q=clear_database_after_test&type=code

But many people use clear_database, which is an alias, as you mention https://github.com/search?q=clear_database+aiida&type=code

I can personally see both stand points (pro/contra removal), and I would certainly agree that relying on setup information being shared between tests can lead to unexpected behavior. (also, clearing the database is a very fast operation; i.e. performance is only a consideration if the setup takes long).

Since I don't think it's a big issue, perhaps we better don't touch it for the moment...

[sphuber]

Fixed by #5372