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

Refactor and overhaul test_official #4087

Merged
merged 9 commits into from Feb 9, 2024
Merged

Conversation

harshil21
Copy link
Member

@harshil21 harshil21 commented Jan 31, 2024

Closes #3874

Features of the overhauled test_official:

  • Modularized
  • Scraping is now done in the beginning and is conveniently wrapped in python classes, and is frozen since it represents the ground truth.
  • Logging: some logging statements are used in the check_param_type function. If you run pytest with --log-level=DEBUG and that test fails, you'll see the logged statements.
  • Major simplification for check_param_type function. See Refactor and overhaul test_official #4087 (comment) and f5704da 's commit message.
  • Some simplification in other places and more documentation

If you see any more areas of improvement, feel free to mention it.

@harshil21 harshil21 changed the title Refactor and overhaul test official Refactor and overhaul test_official Jan 31, 2024
@Bibo-Joshi
Copy link
Member

I scrolled through the changes and didn't see anything that made me want to leave a comment. IISC much of the changes are reorderings, additional comments & extraction of methods, so I've seen most of the code before as well. - Which is ofc not meant to downplay the work you've put into this PR and the gain in maintenability this brings!

You added the do-not-merge label. Do you want to merge after API 7.0?

@harshil21
Copy link
Member Author

I scrolled through the changes and didn't see anything that made me want to leave a comment. IISC much of the changes are reorderings, additional comments & extraction of methods, so I've seen most of the code before as well. - Which is ofc not meant to downplay the work you've put into this PR and the gain in maintenability this brings!

yeah there are just tiny changes here and there to make it easier to maintain, but the code is largely the same.

You added the do-not-merge label. Do you want to merge after API 7.0?

Yes, and that would also give me time to see if I can simplify / improve the check_param_type function..

@harshil21
Copy link
Member Author

harshil21 commented Feb 5, 2024

Latest commits significantly improve check_param_type function, reducing lines of code for that function by 66.4% (excluding newly added logging code)

As a result of simplification, it also found incorrect type hints which went undetected before. There was also further "modularization" making the code easier to follow.

To be merged after #4089, #4088, and #4034.

Base automatically changed from api-7.0 to master February 8, 2024 16:12
@Bibo-Joshi Bibo-Joshi merged commit 1cf63c2 into master Feb 9, 2024
24 checks passed
@Bibo-Joshi Bibo-Joshi deleted the refactor-test-official branch February 9, 2024 17:12
@Bibo-Joshi
Copy link
Member

Sorry again for the merge mess-up and big kudos for the PR!

@github-actions github-actions bot locked and limited conversation to collaborators Feb 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor test_official to be more modular
2 participants