refactor: refactor the `Result` Class #7176

YongGoose · 2025-02-22T05:07:58Z

I have registered the PR changes.

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

fixes #7158

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov · 2025-02-22T05:58:56Z

Codecov Report

Attention: Patch coverage is 41.50943% with 31 lines in your changes missing coverage. Please review.

Project coverage is 51.58%. Comparing base (efa341a) to head (d0cd289).
Report is 3 commits behind head on 2.x.

Files with missing lines	Patch %	Lines
...ache/seata/namingserver/manager/NamingManager.java	12.50%	14 Missing ⚠️
...eata/namingserver/controller/NamingController.java	30.00%	7 Missing ⚠️
...ata/server/controller/VGroupMappingController.java	0.00%	4 Missing ⚠️
...seata/server/console/impl/AbstractLockService.java	0.00%	2 Missing ⚠️
...che/seata/server/controller/ClusterController.java	0.00%	2 Missing ⚠️
...in/java/org/apache/seata/common/result/Result.java	50.00%	1 Missing ⚠️
...eata/namingserver/controller/HealthController.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##                2.x    #7176   +/-   ##
=========================================
  Coverage     51.58%   51.58%           
- Complexity     6815     6818    +3     
=========================================
  Files          1169     1169           
  Lines         41505    41487   -18     
  Branches       4857     4858    +1     
=========================================
- Hits          21409    21402    -7     
+ Misses        18073    18061   -12     
- Partials       2023     2024    +1

Files with missing lines	Coverage Δ
...main/java/org/apache/seata/common/result/Code.java	`86.66% <100.00%> (-2.81%)`	⬇️
...ava/org/apache/seata/common/result/PageResult.java	`87.71% <100.00%> (ø)`
...a/org/apache/seata/common/result/SingleResult.java	`100.00% <100.00%> (+15.38%)`	⬆️
...in/java/org/apache/seata/common/result/Result.java	`77.77% <50.00%> (+24.44%)`	⬆️
...eata/namingserver/controller/HealthController.java	`50.00% <0.00%> (ø)`
...seata/server/console/impl/AbstractLockService.java	`9.09% <0.00%> (ø)`
...che/seata/server/controller/ClusterController.java	`8.88% <0.00%> (+0.19%)`	⬆️
...ata/server/controller/VGroupMappingController.java	`11.76% <0.00%> (+2.24%)`	⬆️
...eata/namingserver/controller/NamingController.java	`28.57% <30.00%> (-3.25%)`	⬇️
...ache/seata/namingserver/manager/NamingManager.java	`49.39% <12.50%> (-0.81%)`	⬇️

... and 1 file 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.

slievrly · 2025-02-22T15:41:00Z

common/src/main/java/org/apache/seata/common/result/Result.java


    public Result() {
+        this(SUCCESS.code, SUCCESS.msg);


An implicit success isn't a great design choice because it doesn't align with the conventional understanding of a zero value. If it needs to represent success, it's better to make it clear in the method name rather than assigning it to a default constructor.

Good catch :)
Done in 3b4370f !

YongGoose · 2025-02-24T07:18:15Z

namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java

-        Result<String> res = createGroup(namespace, vGroup, clusterName, unitName);
+        SingleResult<Void> res = createGroup(namespace, vGroup, clusterName, unitName);


One of the refactorings I made was distinguishing between setting the message of Result as a String and making the generic type of Result a String—these are separate concerns.

https://docs.oracle.com/javase/tutorial/java/generics/types.html

I understand that there are many instances of this pattern in the code.
I plan to focus on refactoring this part as well.

slievrly · 2025-03-06T15:17:56Z

@YongGoose some UT cases failed.
https://github.com/apache/incubator-seata/actions/runs/13701955489/job/38317801470?pr=7176

YongGoose · 2025-03-07T00:15:48Z

@YongGoose some UT cases failed. https://github.com/apache/incubator-seata/actions/runs/13701955489/job/38317801470?pr=7176

I also saw the UT cases failed.
However, I can’t figure out why the test is failing.

To explain in more detail, I can see that testIsSuccess in ResultTest has failed.

org.opentest4j.AssertionFailedError: expected: but was:
at org.apache.seata.common.result.ResultTest.testIsSuccess(ResultTest.java:36)

This test checks the isSuccess method of the Result class, verifying whether the code consists of the string 200.

public boolean isSuccess() {
    return this.code.equals(SUCCESS.code);
}

---

SUCCESS("200", "ok"),

When checking the unit test, I can confirm that the values 200 and ok are being set correctly.

@Test
void testIsSuccess() {
    Result result = new Result("200", "ok");
    Assertions.assertTrue(result.isSuccess());
}

@slievrly What do you think?

PS. please assign below issue to me :) I'll do my best to implement this issue

Enhance E2E Testing in seata-samples with Multiple Environment Combinations #7178

And I’m interested in your mission among Seata’s GSOC missions. 🙂

slievrly · 2025-03-08T13:51:37Z

@YongGoose some UT cases failed. https://github.com/apache/incubator-seata/actions/runs/13701955489/job/38317801470?pr=7176

I also saw the UT cases failed. However, I can’t figure out why the test is failing.

To explain in more detail, I can see that testIsSuccess in ResultTest has failed.

org.opentest4j.AssertionFailedError: expected: but was:
at org.apache.seata.common.result.ResultTest.testIsSuccess(ResultTest.java:36)

This test checks the isSuccess method of the Result class, verifying whether the code consists of the string 200.
public boolean isSuccess() {
    return this.code.equals(SUCCESS.code);
}

---

SUCCESS("200", "ok"),
When checking the unit test, I can confirm that the values 200 and ok are being set correctly.
@Test
void testIsSuccess() {
    Result result = new Result("200", "ok");
    Assertions.assertTrue(result.isSuccess());
}
@slievrly What do you think?

It was a little confusing. I can run your PR code locally and it will pass the unit test. Maybe it's not the code itself, and I'll try to pinpoint the cause.

PS. please assign below issue to me :) I'll do my best to implement this issue

Enhance E2E Testing in seata-samples with Multiple Environment Combinations #7178

I have already commented on this issue.

And I’m interested in your mission among Seata’s GSOC missions. 🙂

Are you talking about this GSoC project? https://cwiki.apache.org/confluence/display/COMDEV/GSoC+2025+Ideas+list#GSoC2025Ideaslist-GSoC2025-ApacheSeata(Incubating)EnhancingConnectionPoolManagementforApacheSeataAT/XATransactionModes

YongGoose · 2025-03-08T13:56:52Z

Are you talking about this GSoC project? https://cwiki.apache.org/confluence/display/COMDEV/GSoC+2025+Ideas+list#GSoC2025Ideaslist-GSoC2025-ApacheSeata(Incubating)EnhancingConnectionPoolManagementforApacheSeataAT/XATransactionModes

Yes, I'm interested in this issue.

slievrly · 2025-03-08T14:49:43Z

Are you talking about this GSoC project? https://cwiki.apache.org/confluence/display/COMDEV/GSoC+2025+Ideas+list#GSoC2025Ideaslist-GSoC2025-ApacheSeata(Incubating)EnhancingConnectionPoolManagementforApacheSeataAT/XATransactionModes

Yes, I'm interested in this issue.

GSoC project has to apply for conditions to open source mainly for beginner or student, specific refer to: https://developers.google.cn/open-source/gsoc/faq

If you meet the requirements for registration, you are welcome to apply.

In addition, the projects you see on this page are currently in idea status, and ASF needs to further approve which projects will eventually participate in GSoC.

slievrly · 2025-03-08T15:42:48Z

@YongGoose some UT cases failed. https://github.com/apache/incubator-seata/actions/runs/13701955489/job/38317801470?pr=7176

I also saw the UT cases failed. However, I can’t figure out why the test is failing.
To explain in more detail, I can see that testIsSuccess in ResultTest has failed.

org.opentest4j.AssertionFailedError: expected: but was:
at org.apache.seata.common.result.ResultTest.testIsSuccess(ResultTest.java:36)

This test checks the isSuccess method of the Result class, verifying whether the code consists of the string 200.
public boolean isSuccess() {
    return this.code.equals(SUCCESS.code);
}

---

SUCCESS("200", "ok"),
When checking the unit test, I can confirm that the values 200 and ok are being set correctly.
@Test
void testIsSuccess() {
    Result result = new Result("200", "ok");
    Assertions.assertTrue(result.isSuccess());
}
@slievrly What do you think?
It was a little confusing. I can run your PR code locally and it will pass the unit test. Maybe it's not the code itself, and I'll try to pinpoint the cause.

https://github.com/apache/incubator-seata/blob/2.x/common/src/test/java/org/apache/seata/common/code/CodeTest.java#L46

slievrly

LGTM

YongGoose added 2 commits February 22, 2025 14:06

refactor codes

aaf8e2a

Add changes

49d81e8

YongGoose force-pushed the feature/7158 branch from f198304 to ccb6634 Compare February 22, 2025 05:41

fix checkstyle

0c848e9

YongGoose force-pushed the feature/7158 branch from ccb6634 to 0c848e9 Compare February 22, 2025 05:46

YongGoose mentioned this pull request Feb 22, 2025

Refactor: Refactoring the Result Class #7158

Closed

YongGoose added 2 commits February 22, 2025 15:07

Rename code

6dfc678

Add test for Result class

7d72ec0

slievrly reviewed Feb 22, 2025

View reviewed changes

YongGoose added 2 commits February 23, 2025 08:16

Remove success state from default constructor

3b4370f

Merge branch '2.x' into feature/7158

bc01acb

YongGoose requested a review from slievrly February 23, 2025 00:42

funky-eyes and others added 2 commits February 24, 2025 14:35

Merge branch '2.x' into feature/7158

17befaf

Merge branch '2.x' into feature/7158

681898e

YongGoose commented Feb 24, 2025

View reviewed changes

YongGoose and others added 4 commits February 24, 2025 18:32

Resolve conflict

c8d355a

Merge branch '2.x' into feature/7158

c93089f

Merge branch '2.x' into feature/7158

d4f50d9

Merge branch '2.x' into feature/7158

a549484

YongGoose added 2 commits March 9, 2025 08:48

Merge branch '2.x' into feature/7158

3a830c7

remove setter in enum class

d0cd289

slievrly approved these changes Mar 9, 2025

View reviewed changes

slievrly changed the title ~~refactor: Refactoring the Result Class~~ refactor: refactor the Result Class Mar 9, 2025

slievrly merged commit 01877a9 into apache:2.x Mar 9, 2025
7 checks passed

slievrly added this to the 2.4.0 milestone Mar 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: refactor the `Result` Class #7176

refactor: refactor the `Result` Class #7176

YongGoose commented Feb 22, 2025

codecov bot commented Feb 22, 2025 •

edited

Loading

slievrly Feb 22, 2025

YongGoose Feb 22, 2025

YongGoose Feb 24, 2025

slievrly commented Mar 6, 2025 •

edited

Loading

YongGoose commented Mar 7, 2025

slievrly commented Mar 8, 2025

YongGoose commented Mar 8, 2025

slievrly commented Mar 8, 2025

slievrly commented Mar 8, 2025

slievrly left a comment

		Result<String> res = createGroup(namespace, vGroup, clusterName, unitName);
		SingleResult<Void> res = createGroup(namespace, vGroup, clusterName, unitName);

refactor: refactor the Result Class #7176

refactor: refactor the Result Class #7176

Conversation

YongGoose commented Feb 22, 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

codecov bot commented Feb 22, 2025 • edited Loading

Codecov Report

slievrly Feb 22, 2025

Choose a reason for hiding this comment

YongGoose Feb 22, 2025

Choose a reason for hiding this comment

YongGoose Feb 24, 2025

Choose a reason for hiding this comment

slievrly commented Mar 6, 2025 • edited Loading

YongGoose commented Mar 7, 2025

slievrly commented Mar 8, 2025

YongGoose commented Mar 8, 2025

slievrly commented Mar 8, 2025

slievrly commented Mar 8, 2025

slievrly left a comment

Choose a reason for hiding this comment

refactor: refactor the `Result` Class #7176

refactor: refactor the `Result` Class #7176

codecov bot commented Feb 22, 2025 •

edited

Loading

slievrly commented Mar 6, 2025 •

edited

Loading