8000 Rework test_runner.py by rdesparbes · Pull Request #281 · CEXT-Dan/PyRx · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Rework test_runner.py #281

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 18, 2025
Merged

Rework test_runner.py #281

merged 2 commits into from
Apr 18, 2025

Conversation

rdesparbes
Copy link
Collaborator
@rdesparbes rdesparbes commented Apr 18, 2025 •
edited
Loading

Context

During my work adding/fixing types for mypy to pass, I came accros the test_runner module, which needed a more thorough rework for mypy to be happy. This rework implies some changes in symbols that are technically public, and in the way the test runner is instantiated. I decided to extract this part to its own PR so that we can discuss with @gswifort and @CEXT-Dan if those changes seem OK.

Problems observed

  • FileArgsTestRunner and CmdlineArgsTestRunner are instantiated in test_test_runner.py even though they are still abstract classes: their run_tests method is not implemented.
  • Use of a type-unsafe getattr(self, "_test_args", ()), where _test_args is not defined in the constructor.
  • More subjective: the multiple inheritance is a bit hard to follow, and the notions of "test runner" and "ways to get the runner arguments" are coupled. This is observable in the names of the classes PytestFileArgsTestRunner and PytestCmdlineArgsTestRunner, where both concepts are tied together.

This pull request replaces multiple inheritance by dependency injection (composition) in order to solve those problems.

To verify that the mypy errors have been correctly fixed, run on main (d545ba7) and rework-test-runner (358da49):

mypy --follow-imports skip pyrx\utils\test_runner.py PySamples/tests/sample_project/run_tests.py tests/test_utils/test_test_runner.py in_test_runner.py

@rdesparbes
Copy link
Collaborator Author

I'll merge main into my branch right after #280 is accepted to make the CI pass

@CEXT-Dan CEXT-Dan merged commit dad3c2f into CEXT-Dan:main Apr 18, 2025
1 check failed
@rdesparbes
Copy link
Collaborator Author

No discussion? 😂

@gswifort
Copy link
Collaborator

I didn't run it, but after reviewing the code it's ok - more readable :)

@CEXT-Dan
Copy link
Owner

No discussion? 😂

I didn’t have a hand in the test runner, I did run the tests a few times today and they are working great from my perspective.

Just an FYI, my python skills are near zero, I read it was easy when I started this project lol. I’m unqualified to critique your code, but equally grateful for your contributions 🏆

@rdesparbes rdesparbes deleted the rework-test-runner branch April 20, 2025 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rdesparbes @gswifort @CEXT-Dan
0