Skip to content

Updated test_utils.py to include the new sid format#41

Merged
ShreyShingala merged 7 commits intomainfrom
fixing-test-utils
Oct 29, 2025
Merged

Updated test_utils.py to include the new sid format#41
ShreyShingala merged 7 commits intomainfrom
fixing-test-utils

Conversation

@ShreyShingala
Copy link
Member

@ShreyShingala ShreyShingala commented Oct 1, 2025

For waterloo-rocketry/2025-software-issues#63

Edited the test_utils.py so that it uses the new SID format as opposed to the previous two variable SID format. Left the old format in comments as it is still used in some test cases, which needs to be fixed.

Also renamed the original file test_utils.py to utils.py, and created accompanying tests in the file utils_test.py

Screenshot 2025-10-14 at 1 09 22 AM

This change is Reviewable

Copy link
Contributor

@ChrisYx511 ChrisYx511 left a comment

Choose a reason for hiding this comment

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

For stacking instructions, see https://abhinav.github.io/git-spice/start/

Copy link
Contributor

@ChrisYx511 ChrisYx511 left a comment

Choose a reason for hiding this comment

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

Also please tag the issue with the PR all the time.

@ShreyShingala
Copy link
Member Author

Got rid of the commented code

@ShreyShingala
Copy link
Member Author

For this issue

@ChrisYx511
Copy link
Contributor

ChrisYx511 commented Oct 3, 2025

@ShreyShingala please include the "For this issue" by editing your original PR description, not in a comment. Also, it would be great if you didn't mask the link. 😃

Edit: I've added it!

Copy link
Contributor

@ChrisYx511 ChrisYx511 left a comment

Choose a reason for hiding this comment

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

Rename the file from test_utils to just utils.py, it is confusing as this is a utility and not a test.
Also, because this is a utility, the utility should also have accompanying unit tests in a utils_test.py file.

Please rename and add accompanying unit tests before requesting a re-review.

@ShreyShingala ShreyShingala marked this pull request as draft October 9, 2025 14:36
@ShreyShingala ShreyShingala marked this pull request as ready for review October 14, 2025 05:12
ChrisYx511
ChrisYx511 previously approved these changes Oct 26, 2025
Copy link
Contributor

@ChrisYx511 ChrisYx511 left a comment

Choose a reason for hiding this comment

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

Generally LGTM!! 🎉

ChrisYx511
ChrisYx511 previously approved these changes Oct 28, 2025
@ShreyShingala ShreyShingala merged commit ee64b72 into main Oct 29, 2025
2 of 3 checks passed
@ShreyShingala ShreyShingala deleted the fixing-test-utils branch October 29, 2025 00:10
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