Skip to content

Testing: fix unintended interactions between tests, part 2#16429

Merged
scheibelp merged 5 commits intospack:developfrom
scheibelp:bugfix/test-state-pollution
May 7, 2020
Merged

Testing: fix unintended interactions between tests, part 2#16429
scheibelp merged 5 commits intospack:developfrom
scheibelp:bugfix/test-state-pollution

Conversation

@scheibelp
Copy link
Copy Markdown
Member

#16003 addressed some unintended interference between unit tests. It appears that #16221 was merged during this time and introduces new unit tests which don't interact well with the fixes. This will reintroduce all changes from #16003 plus whatever is needed to get those changes working with #16221

For now this is just the original changes (and I have marked this WIP). But I think it will be useful to create this thread to track the issue.

@scheibelp scheibelp added the WIP label May 2, 2020
@scheibelp
Copy link
Copy Markdown
Member Author

scheibelp commented May 2, 2020

From #16420 (comment)

It seems the failing test has been added in #16221

The test test_compiler_bootstrap uses the install_mockery and mutable_config fixtures. The install_mockery fixture uses the config fixture. I suspect these two are in conflict (In particular since #16003 updated the mutable_config fixture to actually generate configuration).

Updating install_mockery to use mutable_config instead of config fix the tests that fail, but causes one other new test to fail (which it shouldn't). Either way a softer touch is probably worthwhile (vs. forcing all tests using the install_mockery fixture to use mutable_config).

scheibelp added 4 commits May 4, 2020 12:21
… fixture) and the config fixture resolves test failures but is likely too heavy-handed
…l_mockery with a separate install_mockery_mutable_config fixture that is exactly the same as install_mockery but uses the mutable_config fixture to avoid conflicts
@alalazo
Copy link
Copy Markdown
Member

alalazo commented May 5, 2020

Can we code mutable_config so that it depends on config? This will make it possible to override any config use with mutable_config and be sure about the ordering of the fixtures' setup (first config, then mutable_config).

@scheibelp
Copy link
Copy Markdown
Member Author

Can we code mutable_config so that it depends on config? This will make it possible to override any config use with mutable_config and be sure about the ordering of the fixtures' setup (first config, then mutable_config).

Are you sure PyTest fixtures work like this? I tried updating the mutable_config fixture like so:

def mutable_config(config, tmpdir_factory, configuration_dir):

but get the same failures as #16429 (comment). For one, I don't think fixtures can "override" one another. That being said it was still possible that executing config followed by mutable_config (in that order) would get the desired result; having mutable_config request the config fixture would seem to be the way to do that, but as I mention above the same errors result.

That's possibly because the "conflicting" fixtures are referenced indirectly (i.e. config is used via install_mockery) so in fact PyTest wouldn't be able to guarantee an order and so it doesn't try beyond direct references (for example it can't know how to order mutable_config and install_mockery).

I also tried having mutable_config request install_mockery which also did not succeed, which surprised me; that suggests that multiple "layers" of use_configuration don't actually work together. It looks fine though, so I'm not sure why that should be an issue.

Overall, I think that it merits more investigation, but the current approach may be ideal for now since it works and is straightforward (the downside being that it has code duplication).

See also: https://stackoverflow.com/questions/25660064/in-which-order-are-pytest-fixtures-executed

@scheibelp
Copy link
Copy Markdown
Member Author

For now this is defining an alternative to the install_mockery fixture called install_mockery_mutable_config that uses mutable_config instead of config. This has some code duplication and should likely be refactored in the long term. But for reasons outlined in #16429 (comment) this poses some challenges.

For now I'd suggest merging this and I can make it a priority to refactor if we agree that should occur soon.

@scheibelp scheibelp requested a review from alalazo May 5, 2020 22:09
@aweits
Copy link
Copy Markdown
Contributor

aweits commented May 6, 2020

The last time I ran into something like this it was due to the usage of a contextmanager that was not using yield wrapped in try->finally. (with an exception being thrown during whatever was going on during yield) I note that both use_configuration() and use_store() are lacking that...?

@alalazo
Copy link
Copy Markdown
Member

alalazo commented May 6, 2020

I agree to proceed and have a look at this later. 👍

For one, I don't think fixtures can "override" one another.

What I mean is that fixtures may depend on one another, pytest ensures that the initialization order is respected and people can code them so that one can "override" the other. An example of that in our test suite is database and mutable_database.

If mutable_config depends on config and we ensure that:

  1. The initial config state is stored somewhere
  2. mutable_config yields the same configuration as config (but allows it to be changed)
  3. mutable_config restores config state on teardown

Then everything should work cleanly. The one caveat is that a mutable_config coded this way is supposed to be "final" i.e. just used by tests and not by other fixtures (this cannot be enforced in the language, just by practice as far as I know).

@scheibelp
Copy link
Copy Markdown
Member Author

The last time I ran into something like this it was due to the usage of a contextmanager that was not using yield wrapped in try->finally. (with an exception being thrown during whatever was going on during yield) I note that both use_configuration() and use_store() are lacking that...?

I agree that all context managers implemented using the yield logic should wrap with try/finally, but I think that the usual outcome of forgetting that is that test errors cause problems with other tests. The test failure for test_compiler_bootstrap* appears to be something else (and to check, I did just try updating the use_configuration fixture and it did not resolve the issue).

@scheibelp
Copy link
Copy Markdown
Member Author

What I mean is that fixtures may depend on one another, pytest ensures that the initialization order is respected and people can code them so that one can "override" the other.

That makes sense, although I guessed that was your meaning in #16429 (comment), tried it, and got the same failure:

I also tried having mutable_config request install_mockery which also did not succeed, which surprised me; that suggests that multiple "layers" of use_configuration don't actually work together. It looks fine though, so I'm not sure why that should be an issue.

As of now from reading the code it appears to satisfy constraints (1) and (3) from #16429 (comment) - largely on account of the implementation of the use_configuration context manager (defined in conftest). (2) is not satisfied (the default config scope is omitted) although I'm not clear why it is essential.

An example of that in our test suite is database and mutable_database.

I see that is an example of fixtures using one another, but I don't think it has been subjected to the same stress (I don't see any tests using the database and mutable_database fixtures together).

As a side note, I mentioned in #16429 (comment) that it may not be possible to construct an order in all cases based on fixture parameter requests:

That's possibly because the "conflicting" fixtures are referenced indirectly (i.e. config is used via install_mockery) so in fact PyTest wouldn't be able to guarantee an order and so it doesn't try beyond direct references

I'm wrong there since it must form a DAG and therefore has an order. However, I don't know if PyTest makes the effort to determine the topological order from a list of fixtures (accounting for indirect requests).

@alalazo
Copy link
Copy Markdown
Member

alalazo commented May 7, 2020

(2) is not satisfied (the default config scope is omitted) although I'm not clear why it is essential.

This is essential in general terms since if you have mutable_config and foo_fixture depending on config you will be ensured that config is run before the two of them, but there will be no guarantee on the order of one with respect to the other. This is what I infer from practice, even though the docs don't take this case into account explicitly.

So, if mutable_config is run before foo_fixture and doesn't preserve the setup of config by e.g. changing a configuration that is globally visible, then foo_fixture might be setup with a state that is not the one expected (and fail as a consequence).

@scheibelp scheibelp removed the WIP label May 7, 2020
@scheibelp scheibelp merged commit 1ed564a into spack:develop May 7, 2020
alalazo added a commit to alalazo/spack that referenced this pull request Jul 9, 2024
This fixture was introduced in spack#16429, and made
redundant in spack#39024
haampie pushed a commit that referenced this pull request Jul 9, 2024
This fixture was introduced in #16429, and made
redundant in #39024
hariharan-devarajan pushed a commit to hariharan-devarajan/spack that referenced this pull request Jul 10, 2024
FrederickDeny pushed a commit to FrederickDeny/spack that referenced this pull request Aug 26, 2024
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.

3 participants