-
Notifications
You must be signed in to change notification settings - Fork 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
Make new debug client the default, move old one to classic-debug
#5924
Make new debug client the default, move old one to classic-debug
#5924
Conversation
Should we also rename the folder from |
Sure! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #5924 +/- ##
=============================================
+ Coverage 69.43% 69.46% +0.03%
- Complexity 17065 17076 +11
=============================================
Files 1934 1937 +3
Lines 73606 73680 +74
Branches 7540 7539 -1
=============================================
+ Hits 51109 51183 +74
- Misses 19870 19873 +3
+ Partials 2627 2624 -3 ☔ 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.
Overall looks good, I just requested a few corrections to documentation details.
JsonSupport.prettyPrint(exp), | ||
JsonSupport.prettyPrint(actualNode) | ||
) | ||
"Expected '%s' but actual was '%s'".formatted( |
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.
Is this change intentional? The version of assertEquals
taking a Supplier<String>
would defer the string formatting operation unless the assertion failed. The change here will incur the cost on every assertEqualJson
call, even though the resulting string only ever expected to be used in errors. The performance impact is probably light, but it does seem to me like the pre-PR version followed best practices.
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.
Reverted in 46c5373
Co-authored-by: Andrew Byrd <[email protected]>
Summary
As discussed in the dev meeting yesterday, this makes the new debug client the default and moves the old one into a new folder called
classic-debug
.Issue
Relates to #4828