-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[testutil] Add proper test for the functional tests helpers #8484
[testutil] Add proper test for the functional tests helpers #8484
Conversation
This comment has been minimized.
This comment has been minimized.
e42d9e5
to
ee8baf2
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8484 +/- ##
==========================================
+ Coverage 95.81% 95.91% +0.09%
==========================================
Files 174 174
Lines 18334 18365 +31
==========================================
+ Hits 17567 17615 +48
+ Misses 767 750 -17
|
This comment has been minimized.
This comment has been minimized.
ee8baf2
to
c72fcdd
Compare
This comment has been minimized.
This comment has been minimized.
e7d092d
to
9515a70
Compare
This comment has been minimized.
This comment has been minimized.
9515a70
to
31c8ff0
Compare
This comment has been minimized.
This comment has been minimized.
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.
I left some early comments. Note that I find it quite hard to review, feels like a lot of refactoring and actual code changes are included here. Perhaps split the refactoring + fixes and the adding of tests in two separate PRs?
Created #8490 to simplify the review. |
31c8ff0
to
9357f76
Compare
This comment has been minimized.
This comment has been minimized.
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.
Thanks @Pierre-Sassoulas! Wonderful that all of this code is finally getting tested.
I reviewed this commit by commit as just final diff is still quite large. I think it is probably best to go through them commit by commit as well as the comments make more sense then!
Again, love this! Makes it much easier to use this for plugins as well!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Sorry for taking so long to review this! Hard to find the time to get back into this. Think we are getting there though! Love the idea of all of this being actually tested 😄
84520e7
to
a955ed8
Compare
This comment has been minimized.
This comment has been minimized.
Thanks again @Pierre-Sassoulas for all the work that went into this! |
I'm going to cleanup and rebase :) |
I'll be right here to approve and let auto-merge do its magic 😄 |
0ff5d86
to
8f0de6d
Compare
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 8f0de6d |
Type of Changes
Description
Required for #8474, the output update was been broken a lot previously. We don't need the user warning because either we're going to fail with a message telling to update the output or we're already going to update the output.