optimize: optimize common module by slievrly · Pull Request #7721 · apache/incubator-seata

slievrly · 2025-10-21T15:29:26Z

I have read the CONTRIBUTING.md guidelines.
I have registered the PR changes.

Ⅰ. Describe what this PR did

optimize common module

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

YoWuwuuuw

PR Changes requires registration.

common/src/test/java/org/apache/seata/common/util/CollectionUtilsTest.java

common/src/test/java/org/apache/seata/common/util/NetAddressValidatorUtilTest.java

YoWuwuuuw · 2025-11-03T08:26:00Z

common/src/test/java/org/apache/seata/common/util/ReflectionUtilTest.java

+    }
+
+    @Test
+    @EnabledOnJre({JRE.JAVA_8, JRE.JAVA_11})


Why this test is limited to Java 8 and Java 11, and CI builds fail.

Error: Tests run: 28, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.056 s <<< FAILURE! - in org.apache.seata.common.util.ReflectionUtilTest Error: org.apache.seata.common.util.ReflectionUtilTest.testModifyStaticFinalFieldEnhanced Time elapsed: 0.012 s <<< FAILURE!

The higher version of jdk does not allow the reflection of static final fields. This test case has been temporarily removed for now.

…19_03

Copilot

Pull Request Overview

This PR optimizes the common module by significantly improving test coverage and adding documentation to utility classes. The main focus is on adding comprehensive test cases for existing functionality and improving code maintainability through better comments and documentation.

Key Changes

Added extensive test coverage for utility classes (UUIDGenerator, StringUtils, SizeUtil, ReflectionUtil, NumberUtils, NetUtil, etc.)
Replaced deprecated Class.newInstance() with getDeclaredConstructor().newInstance() in BeanUtils and EnhancedServiceLoader
Added JavaDoc documentation to utility classes (ConfigTools, CycleDependencyHandler, IOUtil, LambdaUtils, etc.)
Enhanced error handling by changing silent exception swallowing to logging in IOUtil

Reviewed Changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
common/src/test/java/.../UUIDGeneratorTest.java	Added tests for UUID generation validation, multiple UUID uniqueness, and init parameter validation
common/src/test/java/.../StringUtilsTest.java	Added duplicate test methods for hasLength and hasText (already covered by existing tests)
common/src/test/java/.../SizeUtilTest.java	Added comprehensive tests for size parsing with valid/invalid inputs and boundary conditions
common/src/test/java/.../ReflectionUtilTest.java	Added enhanced tests for reflection operations including field/method access and error cases
common/src/test/java/.../CollectionUtilsTest.java	Added extensive tests for collection utilities with potential test correctness issues
common/src/main/java/.../BeanUtils.java	Replaced deprecated newInstance() with getDeclaredConstructor().newInstance()
common/src/main/java/.../IOUtil.java	Changed exception handling from silent ignore to warning logs
common/src/main/java/.../ConfigTools.java	Added comprehensive JavaDoc documentation for all public methods
common/src/main/java/.../CycleDependencyHandler.java	Added JavaDoc and improved null safety in containsObject method
common/src/main/java/.../EnhancedServiceLoader.java	Replaced deprecated newInstance() and added cached CLASS_LOADER field

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

common/src/test/java/org/apache/seata/common/util/SizeUtilTest.java

Copilot · 2025-11-11T11:19:09Z

common/src/test/java/org/apache/seata/common/util/CollectionUtilsTest.java

+                .containsEntry("", "value2")
+                .containsEntry("key3", "value3")
+                .hasSize(2);


The test assertion on line 539 is incorrect. Looking at the decode logic, when parsing "key1=&=value2", the entry "key1=" will split into ["key1", ""], and the entry "=value2" will split into ["", "value2"]. However, the test expects only 2 entries with keys "" and "key3", which would mean "key1" is missing. The actual result should have 3 entries: ("key1", ""), ("", "value2"), and ("key3", "value3").

Suggested change

.containsEntry("", "value2")

.containsEntry("key3", "value3")

.hasSize(2);

.containsEntry("key1", "")

.containsEntry("", "value2")

.containsEntry("key3", "value3")

.hasSize(3);

key is null or empty, will ignore.

common/src/main/java/org/apache/seata/common/util/IOUtil.java

common/src/test/java/org/apache/seata/common/util/CollectionUtilsTest.java

common/src/test/java/org/apache/seata/common/util/SizeUtilTest.java

common/src/test/java/org/apache/seata/common/util/ReflectionUtilTest.java

…ilsTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

codecov · 2025-11-11T11:42:23Z

Codecov Report

❌ Patch coverage is 63.63636% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.23%. Comparing base (ca68871) to head (e2ddec1).
⚠️ Report is 1 commits behind head on 2.x.

Files with missing lines	Patch %	Lines
...ata/common/metadata/namingserver/MetaResponse.java	0.00%	8 Missing ⚠️
...ache/seata/common/util/CycleDependencyHandler.java	0.00%	1 Missing and 2 partials ⚠️
...in/java/org/apache/seata/common/io/FileLoader.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #7721      +/-   ##
============================================
+ Coverage     69.03%   69.23%   +0.19%     
  Complexity      994      994              
============================================
  Files          1322     1322              
  Lines         50167    50173       +6     
  Branches       5932     5931       -1     
============================================
+ Hits          34635    34735     +100     
+ Misses        12590    12526      -64     
+ Partials       2942     2912      -30

Files with missing lines	Coverage Δ
...a/org/apache/seata/common/holder/ObjectHolder.java	`100.00% <ø> (ø)`
...che/seata/common/loader/EnhancedServiceLoader.java	`81.60% <100.00%> (+0.14%)`	⬆️
...pache/seata/common/loader/ExtensionDefinition.java	`100.00% <100.00%> (+48.38%)`	⬆️
...n/java/org/apache/seata/common/util/BeanUtils.java	`90.66% <100.00%> (-2.39%)`	⬇️
...java/org/apache/seata/common/util/ConfigTools.java	`100.00% <ø> (ø)`
...main/java/org/apache/seata/common/util/IOUtil.java	`75.00% <100.00%> (+5.00%)`	⬆️
...java/org/apache/seata/common/util/LambdaUtils.java	`66.66% <ø> (+33.33%)`	⬆️
...java/org/apache/seata/common/util/StringUtils.java	`92.94% <ø> (+0.64%)`	⬆️
...in/java/org/apache/seata/common/io/FileLoader.java	`68.42% <66.66%> (ø)`
...ache/seata/common/util/CycleDependencyHandler.java	`87.09% <0.00%> (+4.33%)`	⬆️
... and 1 more

... and 20 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

WangzJi · 2025-11-11T11:45:05Z

LGTM

maple525866

LGTM

* optimize: optimize common module * format * fix review * fix review * Update common/src/test/java/org/apache/seata/common/util/CollectionUtilsTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Jiangke Wu <xingfudeshi@gmail.com> Co-authored-by: YoWuwuuuw <2216348784@qq.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

slievrly and others added 4 commits October 21, 2025 23:28

optimize: optimize common module

1441c71

format

3d8b089

Merge branch '2.x' into 2.x_25_10_19_03

f5642cf

Merge branch '2.x' into 2.x_25_10_19_03

ddad124

YoWuwuuuw reviewed Nov 3, 2025

View reviewed changes

slievrly added 3 commits November 11, 2025 19:01

fix review

a323830

Merge remote-tracking branch 'origin/2.x_25_10_19_03' into 2.x_25_10_…

fc8cd73

…19_03

fix review

5d09fd5

slievrly requested a review from Copilot November 11, 2025 11:11

Copilot started reviewing on behalf of slievrly November 11, 2025 11:12 View session

Copilot finished reviewing on behalf of slievrly November 11, 2025 11:14

Copilot AI reviewed Nov 11, 2025

View reviewed changes

slievrly and others added 2 commits November 11, 2025 19:22

Update common/src/test/java/org/apache/seata/common/util/CollectionUt…

b9f6863

…ilsTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Merge branch '2.x' into 2.x_25_10_19_03

37fae54

slievrly added this to the 2.6.0 milestone Nov 11, 2025

Merge branch '2.x' into 2.x_25_10_19_03

e2ddec1

maple525866 approved these changes Nov 12, 2025

View reviewed changes

YoWuwuuuw approved these changes Nov 12, 2025

View reviewed changes

YoWuwuuuw merged commit 1f69f34 into apache:2.x Nov 12, 2025
14 of 16 checks passed

Conversation

slievrly commented Oct 21, 2025

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Uh oh!

YoWuwuuuw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

YoWuwuuuw Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

slievrly Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

slievrly Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

WangzJi commented Nov 11, 2025

Uh oh!

maple525866 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Nov 11, 2025 •

edited

Loading