-
Notifications
You must be signed in to change notification settings - Fork 152
ci: add workflow check for *_test.py naming convention #749
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
Conversation
Signed-off-by: Raja Rathour <[email protected]>
Signed-off-by: Raja Rathour <[email protected]>
|
Hey @exploreriii , It is ready for review now. |
Signed-off-by: Raja Rathour <[email protected]>
Signed-off-by: Raja Rathour <[email protected]>
Signed-off-by: Raja Rathour <[email protected]>
exploreriii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we check if this works to pick up incorrectly named files?
can you add a unit and integration test file (a copy of another unit and integration test) and just name them incorrectly?
…date CI naming check Signed-off-by: Raja Rathour <[email protected]>
|
|
||
| - name: Validate test file naming convention | ||
| run: | | ||
| echo "Checking for test files that don't end with _test.py..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only the integration tests end with test
the unit tests start with test
|
@exploreriii , ready for final review, please check |
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Raja Rathour <[email protected]>
Signed-off-by: Raja Rathour <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Raja-89
So! Turns out this PR for it to work is more advanced than I had initially anticipated - sorry!
in pr-checks.yml, we basically have a permissions restriction that will never let your test workflow run as a pull request to the python sdk main. The reason this is there, is because one workflow (title check) requires a write permission which requires special security handling.
Therefore, my recommendation is to create a separate pr-check for the title check, call it say pr-check-naming-test.yml. This should have looser permissions because it only requires read access.
To test if this worked, I created a pull request to my exploreriii python sdk main (the fork). This then allows it to run as a separate workflow.
Additionally, as mentioned, we currently have tests named all kinds of different things. Therefore, I created a new issue to standardise the tests to end in _test.py. This will allow your approach of checking all files end in _test.py to pick up all the test files.
There is another issue which you have considered - some files in unit/ and integration/ tests are not tests, but helper utility files. These are necessary to be in unit/ and integration/ but should not be included in the test. In summary, I had to exclude a test naming check on all these files.
So to make your PR actually work you need to:
- Remove the test from pr-check and create a new file.
- in your new test workflow, you need to exclude failures if the files that are helper files eg.. init, mock_server, utils_for_test.
- Wait until test files are renamed _test.py and that is merged
- copy and use for inspiration my test workflow here
exploreriii#13 - exploreriii#13
https://github.com/exploreriii/hiero_sdk_python/pull/13/files#diff-01e130e4348bfe2535500bc5e2613cd1a1aeb078a55bee55c1935c3e3871ae77 - actions 4 is outdated, you should use the most updated one which is version 5 and use the commit hash for it .
https://github.com/actions/checkout/releases/tag/v5.0.0 - as its already tested, you can remove the bad name examples.
Hope this helps and is clearer as to why your workflow will not run and will not correctly pick up test name failures.
If you'd like me to complete the PR for you, please let me know, I can do it and have the files ready to go.
|
Let me start fresh upon a new PR then I will let you know , if it doesn't give the desired output |
Fixes #744
Adds CI check enforcing *_test.py naming for unit/integration tests.