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

test_run.py: some QoL tweaks to the test_run script #131

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

stsquad
Copy link

@stsquad stsquad commented Jul 17, 2023

This adds some simple QoL improvements for those trying to run individual tests from the command line.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • [n/a] All added/changed functionality has a corresponding unit/integration
    test.
  • [n/a] All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • [n/a] Any newly added unsafe code is properly documented.

...and use this is a format string. This will help with later
re-factoring.

Signed-off-by: Alex Bennée <[email protected]>
Rather than make developers dive into the json configuration provide a
little introspection command line to list the available tests.

Signed-off-by: Alex Bennée <[email protected]>
If the user specifies any tests on the command line then only run
those.

Signed-off-by: Alex Bennée <[email protected]>
@@ -66,4 +76,5 @@ def retrieve_test_list(config_file=f"{PARENT_DIR}/.buildkite/test_description.js
setattr(TestsContainer, f"test_{name}", test_func)

if not args.list_tests:
unittest.main(verbosity=2)
tests_suite = unittest.TestLoader().loadTestsFromTestCase(TestsContainer)
unittest.TextTestRunner(verbosity=2).run(tests_suite)
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? Why is the if + continue not enough above?

Copy link
Author

Choose a reason for hiding this comment

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

Fun story - I'm not sure. I was running into weird errors where it was erroring out and I couldn't figure out why. I had a chat (http://ix.io/4API) with gpt4 which suggested the change which worked. It's reasoning might be off but it seemed to imply that the unittests had already been parsed meaning the updated attributes didn't take effect. However I don't understand the guts of the unit test framework enough. The loading of the tests_suite seemed sane though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not fully understand why it worked before and no longer does... The GPT explanation does not make sense to me, since the class starts off empty in both cases.

Maybe this is a bit easier?: https://paste.centos.org/view/8c66c3ef. Not sure if the __name__ assignment is a lot better though...

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.

2 participants