optimize: Add some checks to RegistryFactory #7345
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #7345 +/- ##
============================================
+ Coverage 58.41% 58.48% +0.06%
Complexity 535 535
============================================
Files 1272 1272
Lines 45853 45850 -3
Branches 5520 5521 +1
============================================
+ Hits 26785 26815 +30
+ Misses 16546 16520 -26
+ Partials 2522 2515 -7
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
YongGoose
left a comment
YongGoose
left a comment
There was a problem hiding this comment.
It looks good 👍🏻
Left some comments!
And also, If the implementation is created through RegistryType, it would be better to verify that the intended implementation is created (e.g., using assertEquals) rather than using assertNotNull.
| if (processedRegistryTypes.contains(registryType.name())) { | ||
| LOGGER.warn("The duplicate registration center type '{}' was found in the configuration and has been skipped.", registryType.name()); | ||
| continue; |
There was a problem hiding this comment.
I saw a test that verifies a method was called once by checking if instances.size() equals 1.
How about also adding a test to verify that the corresponding log message is properly output?
What do you think?
There was a problem hiding this comment.
Ok, learned and used the method you wrote before in #7315 (it's awesome)
| } | ||
|
|
||
| /** | ||
| * Use reflection to call the buildRegistryServices method |
There was a problem hiding this comment.
It looks like the comment should refer to buildRegistryService instead of buildRegistryServices.
|
Please take a look :) |
| if (StringUtils.isBlank(registryTypeNamesStr)) { | ||
| registryTypeNamesStr = RegistryType.File.name(); | ||
| } | ||
| String[] registryTypeNames = registryTypeNamesStr.split(Constants.REGISTRY_TYPE_SPLIT_CHAR); |
There was a problem hiding this comment.
Why not convert this directly to a TreeSet? This would allow for case-insensitivity and also handle deduplication
There was a problem hiding this comment.
+1
Also, it would be great to add a test case that covers this scenario.
YongGoose
left a comment
YongGoose
left a comment
There was a problem hiding this comment.
We’re almost there!
Just a few more things to add and it should be good to go.
There was a problem hiding this comment.
Since it’s been changed to a TreeSet, it might be good to add a few more test cases
-
When the same
RegistryTypeis provided with different casing (e.g.,File,FILE), does it recognize them as the same?- This could be verified by checking that no log is printed.
-
When more than one RegistryType is provided, does the log output as expected?
-
When a value not defined in RegistryType is passed, is the appropriate exception thrown? (After removing the try-catch block, this test should be added.)
| } | ||
|
|
||
| LOGGER.info("use registry center type: {}", registryTypeName); | ||
| try { |
There was a problem hiding this comment.
This try-catch block could be removed, as it simply catches the exception and rethrows it without adding any additional handling.
Signed-off-by: yongjunhong <[email protected]>
|
I noticed a very minor change was needed, so I opened a PR to your branch. Feel free to ping me when you have questions :) |
Refactor some tests
3 participants
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
#7342
Ⅲ. Why don't you add test cases (unit test/integration test)?
Covers two factory
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews