-
Notifications
You must be signed in to change notification settings - Fork 8.8k
test: Refactored testSetCodeAndMsgUpdatesValuesCorrectly to use parameterized unit testing #7288
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
base: 2.x
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #7288 +/- ##
============================================
- Coverage 54.52% 54.51% -0.01%
Complexity 7303 7303
============================================
Files 1178 1178
Lines 41962 41962
Branches 4923 4923
============================================
- Hits 22878 22874 -4
- Misses 16914 16923 +9
+ Partials 2170 2165 -5 🚀 New features to boost your workflow:
|
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.
LGTM
@Monilnarang Could you resolve all the conflicts of the PRS you submitted? |
Thanks for the review @slievrly. |
Co-authored-by: Yongjun Hong <[email protected]>
Co-authored-by: Yongjun Hong <[email protected]>
code.setCode(newCode); | ||
code.setMsg(newMsg); |
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.
IMHO, Enums
are generally used for immutable
objects.
If we use a setter on something like Code.SUCCESS
as shown in the code below—setting it to 500 and Server error—it can create confusion for developers.
Code success = Code.SUCCESS;
success.setCode("500");
success.setMsg("Server error");
Therefore, while it's not directly related to this PR, I think it would be better to remove the setter from Code
in a separate PR.
@xingfudeshi What do you think?
Ⅰ. Describe what this PR did
Summary:
testSetCodeAndMsgUpdatesValuesCorrectly
and added more test cases.Elaboration:
getCode
&getMsg()
) would be repeated multiple times with different inputs, making the test harder to maintain and extend.To accomplish this, I have retrofitted the test into a parameterized unit test. This reduces duplication, allows easy extension by simply adding new value sets, and makes debugging easier as it clearly indicates which test failed instead of requiring a search through individual assertions.
Test execution results after changes:

Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Successful tests run
Ⅴ. Special notes for reviews