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

Convert to pytest #1982

Closed
wants to merge 9 commits into from
Closed

Conversation

UkuLoskit
Copy link

@UkuLoskit UkuLoskit commented Jul 10, 2024

There is an issue with the usbprotocol tests, they depend on a particular ordering of the tests. I used pytest-order to preserve the default ordering of unittest. Additionally, these tests cannot be run in parallel. Used this ordering as a crutch to work around this.

@maxime-desroches
Copy link
Contributor

@UkuLoskit Do you plan on finishing this up soon?

@UkuLoskit UkuLoskit marked this pull request as ready for review July 27, 2024 07:36
@UkuLoskit
Copy link
Author

@maxime-desroches hey, ready for review now with some caveats described in the PR description.

@UkuLoskit UkuLoskit marked this pull request as draft July 27, 2024 07:48
@UkuLoskit
Copy link
Author

There are actually more tests ... fixing these

@UkuLoskit UkuLoskit marked this pull request as ready for review July 28, 2024 18:55
@adeebshihadeh
Copy link
Contributor

trigger-jenkins


def test_tx_hook_on_wrong_safety_mode(self):
# No point, since we allow many diagnostic addresses
pass


if __name__ == "__main__":
unittest.main()
pytest.main()
Copy link
Contributor

Choose a reason for hiding this comment

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

the linter should be configured to block this. how is it passing?

@@ -10,6 +10,3 @@ flake8-implicit-str-concat.allow-multiline=false

[tool.ruff.lint.flake8-tidy-imports.banned-api]
"pytest.main".msg = "pytest.main requires special handling that is easy to mess up!"

[tool.pytest.ini_options]
addopts = "-n auto"
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed? we want parallel by default


@pytest.mark.order(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed? the tests should be independent

Copy link
Author

@UkuLoskit UkuLoskit Aug 2, 2024

Choose a reason for hiding this comment

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

@adeebshihadeh I have explained this in PR description The tests are dependent on a definite ordering which be reproduced even without any changes with the unittest package if you change the method names. E.g test_aaa... for the order(5) five test for example. This ordering is just a hack, because fixing the tests to be independent seemed to be a more complicated task that I wanted to skip at least initially

Copy link
Author

Choose a reason for hiding this comment

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

@adeebshihadeh just bumping this since I haven't heard anything back in a while. I think the easiest way to allow parallelism would be to mark the tests that depend on a particular order to run serially. that way parallelism could still be used for other tests. Fixing the tests themselves, seems like a more complicated task.

@adeebshihadeh
Copy link
Contributor

adeebshihadeh commented Aug 27, 2024

Closing, I left feedback and it wasn't addressed. Feel free to open a new one without any "crutches". More background here.

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