Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add stop biz state #1026

Merged
merged 3 commits into from
Nov 13, 2024
Merged

add stop biz state #1026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f34a0d
add stop biz state
lvjing2 Nov 6, 2024
a09dfa0
update ark version
lvjing2 Nov 8, 2024
5e34064
fix test
lvjing2 Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
</modules>

<properties>
<sofa.ark.version>2.2.15</sofa.ark.version>
<sofa.ark.version.old>2.2.14</sofa.ark.version.old>
<sofa.ark.version>2.2.16-SNAPSHOT</sofa.ark.version>
<sofa.ark.version.old>2.2.15</sofa.ark.version.old>
<project.encoding>UTF-8</project.encoding>
<java.version>1.8</java.version>
<license.maven.plugin>3.0</license.maven.plugin>
Expand Down
2 changes: 1 addition & 1 deletion ...arent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ public void stop() {
BizManagerService bizManagerService = ArkServiceContainerHolder.getContainer()
.getService(BizManagerService.class);
bizManagerService.unRegisterBiz(bizName, bizVersion);
setBizState(BizState.UNRESOLVED, StateChangeReason.STOPPED);
setBizState(BizState.STOPPED, StateChangeReason.STOPPED);
eventAdminService.sendEvent(new BeforeBizRecycleEvent(this));
urls = null;
denyImportPackages = null;
Expand Down
6 changes: 3 additions & 3 deletions ...t/core-impl/container/src/test/java/com/alipay/sofa/ark/container/model/BizModelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public void testStopFailedWithClean() {
bizModel.stop();
} catch (RuntimeException e) {
}
assertEquals(BizState.UNRESOLVED, bizModel.getBizState());
assertEquals(BizState.STOPPED, bizModel.getBizState());
}

bizModel.setBizState(BizState.ACTIVATED);
Expand Down Expand Up @@ -186,7 +186,7 @@ public void testStopSucceedWithClean() {
mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer);
bizModel.stop();

assertEquals(BizState.UNRESOLVED, bizModel.getBizState());
assertEquals(BizState.STOPPED, bizModel.getBizState());
}

bizModel.setBizState(BizState.ACTIVATED);
Expand All @@ -201,7 +201,7 @@ public void testStopSucceedWithClean() {
mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer);
bizModel.stop();

assertEquals(BizState.UNRESOLVED, bizModel.getBizState());
assertEquals(BizState.STOPPED, bizModel.getBizState());
} finally {
ArkConfigs.putStringValue(AUTO_UNINSTALL_WHEN_FAILED_ENABLE, "true");
}
Comment on lines +204 to 207
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect constant in cleanup.

The test assertion for the STOPPED state is correct. However, there's a mismatch in the cleanup:

  • The try block sets REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED
  • But the finally block resets AUTO_UNINSTALL_WHEN_FAILED_ENABLE

This could lead to test pollution affecting other tests.

Apply this fix:

         } finally {
-            ArkConfigs.putStringValue(AUTO_UNINSTALL_WHEN_FAILED_ENABLE, "true");
+            ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "true");
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assertEquals(BizState.STOPPED, bizModel.getBizState());
} finally {
ArkConfigs.putStringValue(AUTO_UNINSTALL_WHEN_FAILED_ENABLE, "true");
}
assertEquals(BizState.STOPPED, bizModel.getBizState());
} finally {
ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "true");
}

Expand Down
15 changes: 10 additions & 5 deletions sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@
*/
public enum BizState {
/**
* init but not start install yet
* or
* uninstalled
* not init or not start install yet
*/
UNRESOLVED("unresolved"),
/**
Expand All @@ -45,9 +43,14 @@
DEACTIVATED("deactivated"),

/**
* install failed.
* install or uninstall failed.
*/
BROKEN("broken");
BROKEN("broken"),

/**
* uninstall succeed
*/
STOPPED("stopped");

private String state;

Expand All @@ -73,6 +76,8 @@
return ACTIVATED;
} else if (DEACTIVATED.name().equalsIgnoreCase(state)) {
return DEACTIVATED;
} else if (STOPPED.name().equalsIgnoreCase(state)) {
return STOPPED;

Check warning on line 80 in sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java

View check run for this annotation

Codecov / codecov/patch

sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java#L80 

Added line #L80 was not covered by tests
Comment on lines +79 to +80
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add test coverage for STOPPED state handling.

The static analysis indicates that the STOPPED state handling in the of() method lacks test coverage. This is a critical path that should be tested.

Would you like me to help generate test cases for the following scenarios?

  1. Converting string "STOPPED" to BizState.STOPPED
  2. Case-insensitive handling ("stopped", "STOPPED", "Stopped")
  3. State transition to STOPPED
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 80-80: sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java#L80
Added line #L80 was not covered by tests

} else {
return BROKEN;
}
Expand Down
Loading