-
-
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
[logging-too-many-args] Add functional tests for each value of logging-format-style
#9121
[logging-too-many-args] Add functional tests for each value of logging-format-style
#9121
Conversation
16d4eba
to
f118b37
Compare
This comment has been minimized.
This comment has been minimized.
@Pierre-Sassoulas Do you have more WIP here? Or shall we close this? |
Creating regression tests is work too, why throw it away ? But, I don't have the fix atm no. |
All right the I was on mobile and didn't see the code. Now that I read it and the context the intent of the MR is due to this : #9118 (comment) It was hard to reproduce and the current test is not testing each possibility with each values of |
logging-format-style
I do think we need to explain why we are mixing old and new style tests in a file that has a docstring of "Tests for logging-too-many-args using old style". That will definitely confuse us later on. |
It's confusing me right now π |
f118b37
to
5d8cc88
Compare
5d8cc88
to
d213a89
Compare
d213a89
to
953d3c9
Compare
Updated the docstring for clarity, this is ready for review now. |
Thanks for the review and thanks for the general cleanup earlier Daniel. I did not check everything but I'm not sure we should loose hope on the "need takes over" label and close them all when they age. Some contributors are giving up really close to completion, there's some value in those MR (but we should close low value or unrecoverable one for sure). |
Yeah, those that were close or more recent I kept open. Some of the more controversial ones or low effort ones I closed! |
953d3c9
to
39038b0
Compare
Sorry the new comment moved the whole functional test output, had to regenerate π |
Codecov ReportAll modified and coverable lines are covered by tests β
Additional details and impacted files@@ Coverage Diff @@
## main #9121 +/- ##
=======================================
Coverage 95.82% 95.82%
=======================================
Files 174 174
Lines 18810 18811 +1
=======================================
+ Hits 18024 18025 +1
Misses 786 786
|
π€ According to the primer, this change has no effect on the checked open source code. π€π This comment was generated for commit 39038b0 |
Type of Changes
Description
Refs #9118