Cleanup test side effects #5074
Conversation
TheRealFalcon
force-pushed
the
cleanup-leaked-mocks
branch
from
7e87096 to
908e19b
Compare
There was a problem hiding this comment.
LGTM. Thanks for this effort!
Do you think it is worth creating a scheduled GH action exercising pytest-randomly and pytest-bisect-tests?
I agree with @aciba90. Definitely does not need to be a PR action, but would be good to catch unit tests issues sooner. |
| @@ -763,7 +763,7 @@ def convert_net_json(network_json=None, known_macs=None): | |||
| cfg["type"] = "infiniband" | |||
|
|
|||
| for service in services: | |||
| cfg = service | |||
| cfg = copy.deepcopy(service) | |||
There was a problem hiding this comment.
what necessitated this change and all the other deepcopy calls throughout the unit tests? just curious because I don't see how this related to leaked/failed mocks?
There was a problem hiding this comment.
services is defined earlier as network_json.get("services", []) and network_json was directly passed into the function. That means that later in this loop when we do cfg.update({"type": "nameserver"}) that the passed in network_json gets modified inline.
This fixes that. While that is a bug in and of itself, it was found because we have tests that will pass the same networking config to this function, and so it was modified here in one test causing the test following it to fail.
There was a problem hiding this comment.
With the copy issues, it's not necessarily about mocks and instead about causing side effects that effect other tests. I updated the PR title to reflect this.
There was a problem hiding this comment.
thank you! that makes more sense now.
Yes, I'd like to have a daily that does that along with running unit tests on another platform to further ensure things are mocked enough. |
Mocking doesn't work when we import functions directly.
These are the failures I could find when running tests in random order. Changes that aren't self-explanatory should include a comment in the test itself. test_vultr.py includes a refactor to remove global state and remove a test that didn't work and wasn't needed after the changes.
TheRealFalcon
force-pushed
the
cleanup-leaked-mocks
branch
from
908e19b to
c436a1c
Compare
Mocking doesn't work when we import functions directly.
These are the failures I could find when running tests in random order. Changes that aren't self-explanatory should include a comment in the test itself. test_vultr.py includes a refactor to remove global state and remove a test that didn't work and wasn't needed after the changes.
@TheRealFalcon when you get a chance, I've proposed an Alpine ci PR for this. |
These are the failures I could find when running tests in random order. Changes that aren't self-explanatory should include a comment in the test itself. test_vultr.py includes a refactor to remove global state and remove a test that didn't work and wasn't needed after the changes.
Mocking doesn't work when we import functions directly.
These are the failures I could find when running tests in random order. Changes that aren't self-explanatory should include a comment in the test itself. test_vultr.py includes a refactor to remove global state and remove a test that didn't work and wasn't needed after the changes.
Proposed Commit Message
Additional Context
Issues were found using a combination of
https://pypi.org/project/pytest-randomly/
and
https://pypi.org/project/pytest-bisect-tests/
Merge type