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

Fix a flaky test by cleaning a polluted state. #42

Merged

Conversation

sturmianseq
Copy link
Contributor

What is the purpose of the change

This PR is to fix a flaky test tests/test_assoccomm.py::test_eq_assoccomm, which can fail after running tests/test_assoccomm.py::test_assoccomm_objects, but passes when it is run in isolation.

Reproduce the test failure

Run the following command:

python -m pytest tests/test_assoccomm.py::test_assoccomm_objects tests/test_assoccomm.py::test_eq_assoccomm

Expected Result

tests/test_assoccomm.py::test_eq_assoccomm should pass after running tests/test_assoccomm.py::test_assoccomm_objects.

Actual Result

FAILED tests/test_assoccomm.py::test_eq_assoccomm - TypeError: argument of type 'bool' is not iterable

Why it fails

facts is not reset after running tests/test_assoccomm.py::test_assoccomm_objects with bool remaining.

Fix

Clear associative.facts before tests/test_assoccomm.py::test_eq_assoccomm.

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out!

Just to make sure we're completely clear of the issue, let's use a pytest.fixture (and remove all manual *.clear() calls).

Something like this should work:

@pytest.fixture(autouse=True)
def clear_assoccomm():
    commutative.index.clear()
    commutative.facts.clear()
    associative.index.clear()
    associative.facts.clear()
    yield

Do you mind rebasing with a change to that effect?

@brandonwillard brandonwillard added the bug Something isn't working label Nov 15, 2021
@sturmianseq
Copy link
Contributor Author

Hi @brandonwillard, I changed the patch, please review the code changes, thank you!

brandonwillard
brandonwillard previously approved these changes Nov 16, 2021
@brandonwillard
Copy link
Member

brandonwillard commented Nov 17, 2021

There were some pre-commit issues, so I fixed those and changed the fixture so that it essentially restores the original Relations after each test.

To avoid pre-commit issues in the future, don't forget to set it up in your local environment (see here).

@brandonwillard
Copy link
Member

Also, I noticed that the commit doesn't reference your @sturmianseq account. If you want to change that, you can pull and then modify/rebase the commit. If you don't want to change it, tell me and I'll merge this.

brandonwillard
brandonwillard previously approved these changes Nov 17, 2021
@sturmianseq
Copy link
Contributor Author

Thanks for the reminder! I changed the author, please merge it.

@brandonwillard brandonwillard merged commit cea164f into pythological:master Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants