-
Notifications
You must be signed in to change notification settings - Fork 46
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
editoast: refactor Infra
tests part 1
#7557
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #7557 +/- ##
============================================
- Coverage 29.35% 29.00% -0.36%
- Complexity 2014 2021 +7
============================================
Files 1200 1207 +7
Lines 147406 147365 -41
Branches 2894 2909 +15
============================================
- Hits 43276 42747 -529
- Misses 102426 102903 +477
- Partials 1704 1715 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Nice PR! Here's a few suggestions:
-
Your first commit message is too long. We try to enforce a 70 character limit.
-
You can squash your second and third commit to avoid commented code in the history.
-
There are many tests that are very similar and duplicated for each object type. A nice improvement would be to write a
macro_rule!
to factorize all that (just liketest_persist!
). -
This would be not only valuable to decrease the amount of code, but also to have fewer SQL requests to migrate when we'll try to eliminate
diesel
outside ofeditoast_models
. -
Speaking of, some requests may be rewritten using ModelV2's API for listing and counting. If you use a macro, the rewrite would occur only in one place ;)
Except for the git
part, none of this is blocking, but it might be worth doing that now while you're working on this part of the code. This way we'll have less work down the line to get a proper models
split :)
bbfe63f
to
6ca8ec6
Compare
6ca8ec6
to
e88af2b
Compare
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.
Good job with the macros! :D
Just a few details and 🚀
21e7bcd
to
edbb371
Compare
Part of #6980