Make backend integration test endpoints configurable#27664
Conversation
📝 WalkthroughOverviewThis pull request enables backend integration tests to run against an external or pre-configured WSO2 Identity Server instance, rather than only supporting the default embedded server setup. Tests can now be pointed to an external server by specifying an alternate automation configuration file and system properties for the target server endpoints. Key ChangesConfiguration & System Properties:
Endpoint Configuration:
External Server Integration:
Documentation:
Usage ExampleTo run integration tests against an external server: Testing Status
WalkthroughThe pull request centralizes test host/port/URL values into CommonConstants (system-property-driven defaults) and updates multiple integration tests to use those constants instead of hardcoded localhost endpoints (SAML, OAuth, PassiveSTS, auth flows). Adds NoOpServerExtension to skip server lifecycle management for externally-run Identity Server instances, plus automation-external.xml, README-EXTERNAL-TESTING.md, and EXTERNAL-TESTING-FIXED.md to document and configure external testing. The Maven profile now exposes an Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant ExtConfig as automation-external.xml
participant NoOpExt as NoOpServerExtension
participant UserPop as UserPopulateExtension
participant ExternalIS as External Identity Server
participant SampleApp as Sample App
TestRunner->>ExtConfig: load external test configuration
TestRunner->>NoOpExt: register extension (no lifecycle actions)
NoOpExt-->>TestRunner: acknowledge external IS expected
TestRunner->>UserPop: invoke user/tenant population
UserPop->>ExternalIS: create test users/tenants
TestRunner->>ExternalIS: execute integration tests (SAML/OAuth/STS/auth)
ExternalIS->>SampleApp: perform redirects/callbacks (ACS/redirect URIs)
SampleApp-->>ExternalIS: application callbacks/responses
ExternalIS-->>TestRunner: return responses/assertions for verification
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
PR builder started |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/apiAuthorization/RBACWithAPIAuthorizationTestCase.java`:
- Line 67: The CALLBACK_URL constant is missing an explicit port and will
default to 443; update its definition (CALLBACK_URL) to include the integration
test HTTPS port by concatenating CommonConstants.IS_DEFAULT_HTTPS_PORT (e.g.,
"https://" + CommonConstants.IS_HOST + ":" +
CommonConstants.IS_DEFAULT_HTTPS_PORT + "/callback") so it matches the pattern
used by IS_HTTPS_BASE_URL and routes correctly in the test environment.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/utils/CommonConstants.java`:
- Around line 52-59: getSystemPropertyAsInt currently calls
Integer.parseInt(value.trim()) which will throw NumberFormatException and
surface as ExceptionInInitializerError; update getSystemPropertyAsInt to catch
NumberFormatException around the parse, and on parse failure return the provided
defaultValue (optionally log the invalid value) so a malformed system property
doesn't abort test initialization; ensure the method still returns the parsed
int on success and references getSystemPropertyAsInt in changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 336bfa9b-d301-4067-97ec-4cf4a82f8cc2
📒 Files selected for processing (10)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/AuthenticationAdminTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/apiAuthorization/RBACWithAPIAuthorizationTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/saml/AbstractSAMLSSOTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/saml/SAMLECPSSOTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/saml/SAMLInvalidIssuerTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/saml/SAMLLocalAndOutboundAuthenticatorsTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/saml/SAMLMetadataTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/saml/SAMLSSOConsentTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/sts/TestPassiveSTS.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/utils/CommonConstants.java
| public class RBACWithAPIAuthorizationTestCase extends OAuth2ServiceAbstractIntegrationTest { | ||
|
|
||
| private static final String CALLBACK_URL = "https://localhost/callback"; | ||
| private static final String CALLBACK_URL = "https://" + CommonConstants.IS_HOST + "/callback"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect git history for the previous CALLBACK_URL value to compare.
git log -n 3 --follow -p -- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/apiAuthorization/RBACWithAPIAuthorizationTestCase.java | rg -n -B1 -A1 'CALLBACK_URL'Repository: wso2/product-is
Length of output: 374
🏁 Script executed:
# Get broader git history to see what the previous CALLBACK_URL definition was
git log --all -p --follow -- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/apiAuthorization/RBACWithAPIAuthorizationTestCase.java | head -200Repository: wso2/product-is
Length of output: 9622
🏁 Script executed:
# Search for CommonConstants.IS_HOST definition to understand what it contains
rg -r "IS_HOST" --type java -B2 -A2 modules/integration/tests-integration/tests-backend/Repository: wso2/product-is
Length of output: 1194
🏁 Script executed:
# Search for any other uses of CALLBACK_URL in the test file to understand how it's used
rg -n "CALLBACK_URL" --type java modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/apiAuthorization/RBACWithAPIAuthorizationTestCase.java -B2 -A2Repository: wso2/product-is
Length of output: 687
🏁 Script executed:
# Find and examine CommonConstants class definition
find . -name "CommonConstants.java" -type f | head -5Repository: wso2/product-is
Length of output: 189
🏁 Script executed:
# Search for getBasicOAuthApp method to understand how CALLBACK_URL is used
rg -n "getBasicOAuthApp" --type java -B3 -A10Repository: wso2/product-is
Length of output: 42428
🏁 Script executed:
# Check for similar callback URL patterns in other test files
rg -n "CALLBACK_URL\|callback" --type java modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/ -B1 -A1 | head -50Repository: wso2/product-is
Length of output: 41
🏁 Script executed:
# Read CommonConstants.java to find IS_HOST definition
cat modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/utils/CommonConstants.java | grep -n "IS_HOST\|IS_HTTPS" -A2 -B2Repository: wso2/product-is
Length of output: 1133
Add explicit port to CALLBACK_URL for consistency with integration test environment.
CALLBACK_URL is missing the port component. The standard pattern in this codebase (e.g., IS_HTTPS_BASE_URL) explicitly includes IS_DEFAULT_HTTPS_PORT (defaulting to 9853). Without an explicit port, the callback URL defaults to 443, not the integration test port. Consider using "https://" + CommonConstants.IS_HOST + ":" + CommonConstants.IS_DEFAULT_HTTPS_PORT + "/callback" to align with other URLs and ensure correct routing during testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/apiAuthorization/RBACWithAPIAuthorizationTestCase.java`
at line 67, The CALLBACK_URL constant is missing an explicit port and will
default to 443; update its definition (CALLBACK_URL) to include the integration
test HTTPS port by concatenating CommonConstants.IS_DEFAULT_HTTPS_PORT (e.g.,
"https://" + CommonConstants.IS_HOST + ":" +
CommonConstants.IS_DEFAULT_HTTPS_PORT + "/callback") so it matches the pattern
used by IS_HTTPS_BASE_URL and routes correctly in the test environment.
There was a problem hiding this comment.
This constant is used to replace the string "localhost" in each reference. So keeping this constant is fine.
There was a problem hiding this comment.
@DMHP, understood — since CALLBACK_URL is used as a registered redirect URI pattern (replacing the localhost literal) rather than as a direct network endpoint, an explicit port is not required here. I'll defer to your judgment on this.
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/24875508234
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/utils/CommonConstants.java`:
- Around line 46-50: getSystemProperty currently only treats null as unset;
change it so that values that are empty or only whitespace are treated as unset
too by calling System.getProperty(key), trimming the result (or using a
null-safe check), and returning defaultValue when the retrieved value is null or
blank; update the getSystemProperty method to trim the value and return
defaultValue if value == null || value.trim().isEmpty() (referencing
getSystemProperty and System.getProperty).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61f3d8cd-eafa-4fcf-9562-5911d4bd6b39
📒 Files selected for processing (1)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/utils/CommonConstants.java
| private static String getSystemProperty(String key, String defaultValue) { | ||
|
|
||
| String value = System.getProperty(key); | ||
| return value != null ? value : defaultValue; | ||
| } |
There was a problem hiding this comment.
Handle blank string properties as default values.
getSystemProperty currently falls back only for null. If a property is set to empty/whitespace, callers receive an unusable value (for example, empty host), which can break derived URLs. Treat blank values as unset.
Suggested fix
private static String getSystemProperty(String key, String defaultValue) {
String value = System.getProperty(key);
- return value != null ? value : defaultValue;
+ if (value == null || value.trim().isEmpty()) {
+ return defaultValue;
+ }
+ return value.trim();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/utils/CommonConstants.java`
around lines 46 - 50, getSystemProperty currently only treats null as unset;
change it so that values that are empty or only whitespace are treated as unset
too by calling System.getProperty(key), trimming the result (or using a
null-safe check), and returning defaultValue when the retrieved value is null or
blank; update the getSystemProperty method to trim the value and return
defaultValue if value == null || value.trim().isEmpty() (referencing
getSystemProperty and System.getProperty).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
modules/integration/tests-integration/tests-backend/README-EXTERNAL-TESTING.md (1)
168-189: Make the CI script exit path deterministic.Initialize
TESTS_FAILEDbefore execution so the finalexitis explicit and predictable.Suggested script refinement
#!/bin/bash set -e +TESTS_FAILED=0 @@ mvn -pl modules/integration/tests-integration/tests-backend test \ -Dautomation.config.file=automation-external.xml \ -Dintegration.test.is.host=${IS_HOST} \ -Dintegration.test.is.https.port=${IS_PORT} \ || TESTS_FAILED=1 @@ -kill $IS_PID +kill $IS_PID || true -exit ${TESTS_FAILED} +exit ${TESTS_FAILED}As per coding guidelines, provide concise, actionable feedback focused on correctness and best practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/README-EXTERNAL-TESTING.md` around lines 168 - 189, The script fails to initialize TESTS_FAILED which makes the final exit code nondeterministic; initialize TESTS_FAILED=0 before starting the external IS (e.g., add TESTS_FAILED=0 near the top before ./start-external-is.sh & and IS_PID assignment) so that the later conditional assignment (|| TESTS_FAILED=1) and final exit ${TESTS_FAILED} are deterministic; ensure you reference the existing symbols TESTS_FAILED, IS_PID, and start-external-is.sh when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/integration/tests-common/integration-test-utils/src/main/java/org/wso2/identity/integration/common/extension/server/NoOpServerExtension.java`:
- Around line 33-43: In the Javadoc for class NoOpServerExtension, the example
block uses raw angle-bracket placeholders which Javadoc may treat as tags; fix
this by wrapping the entire example block in a {`@code` ...} tag and escape any
literal '<' and '>' (or replace them with < and >) inside the {`@code`} so
the placeholders like <class>, <name>, <your-external-is-host> render verbatim;
update the comment block surrounding the example accordingly.
In
`@modules/integration/tests-integration/tests-backend/README-EXTERNAL-TESTING.md`:
- Around line 83-92: Add language identifiers to the two fenced code blocks
shown in the README: change the block that begins with "IS_HOST resolution:" to
use a language tag (e.g., ```text) and change the block that contains
"java.net.ConnectException: Connection refused" to also use a language tag
(e.g., ```text or ```textually-appropriate), so both fenced blocks include a
language identifier to satisfy markdownlint MD040; locate the blocks by their
contents ("IS_HOST resolution:" and "java.net.ConnectException: Connection
refused") and update the opening ``` lines accordingly.
In
`@modules/integration/tests-integration/tests-backend/src/test/resources/automation-external.xml`:
- Around line 52-57: The comment block contains a duplicate property key
integration.test.sample.http.port with a different description; fix it by
removing the duplicate or replacing it with the correct key (e.g.,
integration.test.sample.port or the intended property name) and update the
description to match that key (ensuring the comment for
integration.test.sample.http.port appears only once and the “default if specific
port not set” text is applied to the correct property).
---
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/README-EXTERNAL-TESTING.md`:
- Around line 168-189: The script fails to initialize TESTS_FAILED which makes
the final exit code nondeterministic; initialize TESTS_FAILED=0 before starting
the external IS (e.g., add TESTS_FAILED=0 near the top before
./start-external-is.sh & and IS_PID assignment) so that the later conditional
assignment (|| TESTS_FAILED=1) and final exit ${TESTS_FAILED} are deterministic;
ensure you reference the existing symbols TESTS_FAILED, IS_PID, and
start-external-is.sh when applying the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 161b2a37-c38e-40aa-946f-e3adcbd386fd
📒 Files selected for processing (3)
modules/integration/tests-common/integration-test-utils/src/main/java/org/wso2/identity/integration/common/extension/server/NoOpServerExtension.javamodules/integration/tests-integration/tests-backend/README-EXTERNAL-TESTING.mdmodules/integration/tests-integration/tests-backend/src/test/resources/automation-external.xml
| * Configuration: | ||
| * <class> | ||
| * <name>org.wso2.identity.integration.common.extension.server.NoOpServerExtension</name> | ||
| * </class> | ||
| * | ||
| * System Properties: | ||
| * -Dintegration.test.is.host=<your-external-is-host> | ||
| * -Dintegration.test.is.https.port=<your-external-is-https-port> | ||
| * -Dintegration.test.sample.host=<your-sample-app-host> | ||
| * -Dintegration.test.sample.http.port=<your-sample-app-http-port> | ||
| */ |
There was a problem hiding this comment.
Escape angle-bracket placeholders in Javadoc examples.
On Line 34–42, raw <...> placeholders can be parsed as tags by Javadoc and render incorrectly. Please wrap the example block with {@code ...} and escape placeholder brackets.
Suggested doc fix
- * Configuration:
- * <class>
- * <name>org.wso2.identity.integration.common.extension.server.NoOpServerExtension</name>
- * </class>
+ * Configuration:
+ * {`@code`
+ * <class>
+ * <name>org.wso2.identity.integration.common.extension.server.NoOpServerExtension</name>
+ * </class>
+ * }
*
* System Properties:
- * -Dintegration.test.is.host=<your-external-is-host>
- * -Dintegration.test.is.https.port=<your-external-is-https-port>
- * -Dintegration.test.sample.host=<your-sample-app-host>
- * -Dintegration.test.sample.http.port=<your-sample-app-http-port>
+ * -Dintegration.test.is.host=<your-external-is-host>
+ * -Dintegration.test.is.https.port=<your-external-is-https-port>
+ * -Dintegration.test.sample.host=<your-sample-app-host>
+ * -Dintegration.test.sample.http.port=<your-sample-app-http-port>As per coding guidelines, provide concise, actionable feedback focused on correctness and best practices.
📝 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.
| * Configuration: | |
| * <class> | |
| * <name>org.wso2.identity.integration.common.extension.server.NoOpServerExtension</name> | |
| * </class> | |
| * | |
| * System Properties: | |
| * -Dintegration.test.is.host=<your-external-is-host> | |
| * -Dintegration.test.is.https.port=<your-external-is-https-port> | |
| * -Dintegration.test.sample.host=<your-sample-app-host> | |
| * -Dintegration.test.sample.http.port=<your-sample-app-http-port> | |
| */ | |
| * Configuration: | |
| * {`@code` | |
| * <class> | |
| * <name>org.wso2.identity.integration.common.extension.server.NoOpServerExtension</name> | |
| * </class> | |
| * } | |
| * | |
| * System Properties: | |
| * -Dintegration.test.is.host=<your-external-is-host> | |
| * -Dintegration.test.is.https.port=<your-external-is-https-port> | |
| * -Dintegration.test.sample.host=<your-sample-app-host> | |
| * -Dintegration.test.sample.http.port=<your-sample-app-http-port> | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-common/integration-test-utils/src/main/java/org/wso2/identity/integration/common/extension/server/NoOpServerExtension.java`
around lines 33 - 43, In the Javadoc for class NoOpServerExtension, the example
block uses raw angle-bracket placeholders which Javadoc may treat as tags; fix
this by wrapping the entire example block in a {`@code` ...} tag and escape any
literal '<' and '>' (or replace them with < and >) inside the {`@code`} so
the placeholders like <class>, <name>, <your-external-is-host> render verbatim;
update the comment block surrounding the example accordingly.
| ``` | ||
| IS_HOST resolution: | ||
| 1. Check: System.getProperty("integration.test.is.host") | ||
| ✓ If found, use this value | ||
|
|
||
| 2. If not found, check: System.getProperty("integration.test.host") | ||
| ✓ If found, use this value | ||
|
|
||
| 3. If not found, use default: "localhost" | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
The fenced blocks on Line 83 and Line 110 are missing language labels (markdownlint MD040).
Suggested doc fix
-```
+```text
IS_HOST resolution:
1. Check: System.getProperty("integration.test.is.host")
✓ If found, use this value
@@
3. If not found, use default: "localhost"@@
- +text
java.net.ConnectException: Connection refused
As per coding guidelines, provide concise, actionable feedback focused on correctness and best practices.
Also applies to: 110-112
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/README-EXTERNAL-TESTING.md`
around lines 83 - 92, Add language identifiers to the two fenced code blocks
shown in the README: change the block that begins with "IS_HOST resolution:" to
use a language tag (e.g., ```text) and change the block that contains
"java.net.ConnectException: Connection refused" to also use a language tag
(e.g., ```text or ```textually-appropriate), so both fenced blocks include a
language identifier to satisfy markdownlint MD040; locate the blocks by their
contents ("IS_HOST resolution:" and "java.net.ConnectException: Connection
refused") and update the opening ``` lines accordingly.
| ~ integration.test.is.host - External IS hostname (default: localhost) | ||
| ~ integration.test.is.https.port - External IS HTTPS port (default: 9853) | ||
| ~ integration.test.sample.host - Sample app hostname (default: localhost) | ||
| ~ integration.test.sample.http.port - Sample app HTTP port (default: 8490) | ||
| ~ integration.test.host - Global default hostname (used if specific *is.host not set) | ||
| ~ integration.test.sample.http.port - Sample app default HTTP port (used if specific port not set) |
There was a problem hiding this comment.
Fix duplicated property key in the configuration comment block.
Line 57 repeats integration.test.sample.http.port with a different description, which is confusing during setup. Please correct or remove the duplicate entry.
As per coding guidelines, provide concise, actionable feedback focused on correctness and best practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/resources/automation-external.xml`
around lines 52 - 57, The comment block contains a duplicate property key
integration.test.sample.http.port with a different description; fix it by
removing the duplicate or replacing it with the correct key (e.g.,
integration.test.sample.port or the intended property name) and update the
description to match that key (ensuring the comment for
integration.test.sample.http.port appears only once and the “default if specific
port not set” text is applied to the correct property).
…uration - Added automation.config.file property to both integration and testgrid profiles - Default value is automation.xml to maintain backward compatibility - Can be overridden at runtime: -Dautomation.config.file=automation-external.xml - Updated README-EXTERNAL-TESTING.md with correct Maven goals and troubleshooting - Clarified that 'mvn test' goal must be used (not 'mvn clean install')
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@EXTERNAL-TESTING-FIXED.md`:
- Around line 38-39: Replace the hardcoded absolute path "cd
/Users/hasanthi/Applications/Source/Product_IS/product-is" with a
repository-relative path so the example is portable (e.g., "cd product-is" or
"cd ." from repo root), and keep the following Maven invocation "mvn -pl
modules/integration/tests-integration/tests-backend test \ " unchanged except
ensure any other absolute paths in the same section (lines 96-99) are likewise
converted to repository-relative forms.
- Around line 109-117: Fenced code blocks containing the server log snippets
(e.g., the block starting with "[INFO] NoOpServerExtension: Initiated. No server
lifecycle management will be performed." and the block starting with "ERROR
[org.wso2.carbon.automation.extensions.servers.utils.ArchiveExtractor] - Error
on archive extraction") need language identifiers to satisfy MD040; update each
triple-backtick fence to include "text" (```text) so the two blocks are
explicitly marked as plain text.
- Around line 74-79: Update the incorrect note that claims "mvn clean install -
Does NOT run tests" to state that "mvn clean install" runs the test phase by
default as part of Maven's lifecycle
(validate→compile→test→package→verify→install); also mention how to skip tests
explicitly (e.g., -DskipTests or -Dmaven.test.skip=true) so the example `mvn
clean install -Dautomation.config.file=automation-external.xml` is accurate and
the documentation no longer implies tests are skipped by default.
In
`@modules/integration/tests-integration/tests-backend/README-EXTERNAL-TESTING.md`:
- Around line 126-138: The README contains two identical headings "Error: Server
Extension Still Attempting Auto-Start"; change the second heading (the one above
the note about tests extracting pack from `modules/distribution/target/`) to a
distinct, descriptive heading such as "Error: Tests Extracting Pack from
modules/distribution/target" so each troubleshooting case has a unique title;
update the heading text only (keep surrounding content) to reflect the specific
failure mode and ensure clarity.
- Around line 73-77: Update the README wording to correctly state that Maven
runs tests by default and that mvn clean install includes the test phase (so
replace the incorrect line claiming mvn clean install does not run tests); also
fix the contradictory example that shows mvn clean install -DskipTests test by
either removing -DskipTests or changing the command to a valid combination
(e.g., mvn clean install to run tests, or mvn clean install -DskipTests to skip
them, or use mvn test/mvn clean test where appropriate); ensure the README
examples and explanations consistently state that tests are run by default
unless -DskipTests is provided and adjust the two affected examples/lines
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0098c51-527e-432c-8338-3216acbeef65
📒 Files selected for processing (3)
EXTERNAL-TESTING-FIXED.mdmodules/integration/tests-integration/tests-backend/README-EXTERNAL-TESTING.mdmodules/integration/tests-integration/tests-backend/pom.xml
| cd /Users/hasanthi/Applications/Source/Product_IS/product-is | ||
| mvn -pl modules/integration/tests-integration/tests-backend test \ |
There was a problem hiding this comment.
Use repository-relative paths in examples.
The hardcoded local paths on Line 38 and Line 96 are user-specific. Replace them with generic/project-relative paths to keep docs portable.
Suggested doc update
-cd /Users/hasanthi/Applications/Source/Product_IS/product-is
+cd /path/to/product-is
...
-cd /Users/hasanthi/Applications/Source/Product_IS/product-is
+cd /path/to/product-isAlso applies to: 96-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@EXTERNAL-TESTING-FIXED.md` around lines 38 - 39, Replace the hardcoded
absolute path "cd /Users/hasanthi/Applications/Source/Product_IS/product-is"
with a repository-relative path so the example is portable (e.g., "cd
product-is" or "cd ." from repo root), and keep the following Maven invocation
"mvn -pl modules/integration/tests-integration/tests-backend test \ " unchanged
except ensure any other absolute paths in the same section (lines 96-99) are
likewise converted to repository-relative forms.
| ### ❌ DON'T DO THIS | ||
| ```bash | ||
| mvn clean install -Dautomation.config.file=automation-external.xml | ||
| ``` | ||
| - ❌ `mvn clean install` - Does NOT run tests (install goal doesn't execute tests) | ||
| - ❌ Tests won't run, so the parameter is ignored |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Apache Maven's default lifecycle, does running mvn clean installexecute thetest phase (and surefire tests) by default unless skipped?
💡 Result:
Yes, in Apache Maven's default lifecycle, running mvn clean install executes the test phase (and thus Surefire tests) by default unless explicitly skipped (e.g., via -DskipTests or -Dmaven.test.skip=true). The clean phase runs the clean lifecycle (deleting the target directory). Then install runs the default lifecycle up to and including the install phase, which includes earlier phases in sequence: validate, compile, test, package, verify, install. The test phase is explicitly listed before package and install. The Maven Surefire Plugin's surefire:test goal is bound by default to the test phase, executing unit tests (JUnit/TestNG) and generating reports in target/surefire-reports unless skipped.
Citations:
- 1: https://maven.apache.org/maven2/guides/introduction/introduction-to-the-lifecycle.html
- 2: https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html
- 3: https://maven.apache.org/ref/3.3.1/maven-core/lifecycles.html
- 4: https://stackoverflow.com/questions/10533828/what-does-mvn-install-in-maven-exactly-do
- 5: https://medium.com/@qingedaig/what-does-mvn-clean-install-include-00cfaecd3657
- 6: https://maven.apache.org/maven2/run.html
- 7: https://maven.apache.org/surefire/maven-surefire-plugin/usage.html
- 8: https://maven.apache.org/plugins/maven-surefire-plugin/test-mojo.html
- 9: https://maven.apache.org/surefire/maven-surefire-plugin/examples/skipping-tests.html
- 10: https://maven.apache.org/surefire/maven-surefire-plugin/index.html
Correct the mvn clean install behavior note.
The statement on Line 78 is inaccurate. By default in Apache Maven, mvn clean install executes the test phase as part of the default lifecycle (validate → compile → test → package → verify → install). Tests run automatically unless explicitly skipped with flags like -DskipTests or -Dmaven.test.skip=true. Update the documentation to reflect this correct behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@EXTERNAL-TESTING-FIXED.md` around lines 74 - 79, Update the incorrect note
that claims "mvn clean install - Does NOT run tests" to state that "mvn clean
install" runs the test phase by default as part of Maven's lifecycle
(validate→compile→test→package→verify→install); also mention how to skip tests
explicitly (e.g., -DskipTests or -Dmaven.test.skip=true) so the example `mvn
clean install -Dautomation.config.file=automation-external.xml` is accurate and
the documentation no longer implies tests are skipped by default.
| ``` | ||
| [INFO] NoOpServerExtension: Initiated. No server lifecycle management will be performed. | ||
| [INFO] NoOpServerExtension: onExecutionStart() - No action taken. Expecting external IS to be running. | ||
| ``` | ||
|
|
||
| Not this (which means it's trying to auto-start): | ||
| ``` | ||
| ERROR [org.wso2.carbon.automation.extensions.servers.utils.ArchiveExtractor] - Error on archive extraction | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
The fenced blocks starting at Line 109 and Line 115 should include a language label to satisfy MD040.
Suggested doc update
-```
+```text
[INFO] NoOpServerExtension: Initiated. No server lifecycle management will be performed.
[INFO] NoOpServerExtension: onExecutionStart() - No action taken. Expecting external IS to be running.@@
- +text
ERROR [org.wso2.carbon.automation.extensions.servers.utils.ArchiveExtractor] - Error on archive extraction
📝 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.
| ``` | |
| [INFO] NoOpServerExtension: Initiated. No server lifecycle management will be performed. | |
| [INFO] NoOpServerExtension: onExecutionStart() - No action taken. Expecting external IS to be running. | |
| ``` | |
| Not this (which means it's trying to auto-start): | |
| ``` | |
| ERROR [org.wso2.carbon.automation.extensions.servers.utils.ArchiveExtractor] - Error on archive extraction | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@EXTERNAL-TESTING-FIXED.md` around lines 109 - 117, Fenced code blocks
containing the server log snippets (e.g., the block starting with "[INFO]
NoOpServerExtension: Initiated. No server lifecycle management will be
performed." and the block starting with "ERROR
[org.wso2.carbon.automation.extensions.servers.utils.ArchiveExtractor] - Error
on archive extraction") need language identifiers to satisfy MD040; update each
triple-backtick fence to include "text" (```text) so the two blocks are
explicitly marked as plain text.
| **IMPORTANT: Use `test` goal NOT `clean install`** | ||
| - ❌ `mvn clean install` - This compiles but does NOT run tests | ||
| - ✅ `mvn test` - This compiles and runs tests | ||
| - ✅ `mvn clean test` - This cleans, compiles, and runs tests | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How does Apache Maven execute phases when running mvn clean install, and is testalready included beforeinstall by default?
💡 Result:
When running mvn clean install, Maven first executes the clean lifecycle (phases: pre-clean, clean, post-clean), which removes the target directory and previous build artifacts. Then it executes the install phase of the default lifecycle, which triggers all preceding phases in sequence: validate, compile, test, package, verify, and install. The test phase (bound to surefire:test by default) runs unit tests before package and thus before install. Yes, test is included by default before install.
Citations:
- 1: https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html
- 2: https://www.marcobehler.com/guides/mvn-clean-install-a-short-guide-to-maven
- 3: https://maven.apache.org/run.html
- 4: https://medium.com/@qingedaig/what-does-mvn-clean-install-include-00cfaecd3657
- 5: http://maven.apache.org/ref/3.9.13/maven-core/lifecycles.html
Correct the Maven command guidance; mvn clean install runs tests by default.
The documentation on line 74 incorrectly states that mvn clean install does not run tests. According to Apache Maven's standard lifecycle, this command includes and executes the test phase before the install phase. Tests run by default unless explicitly skipped with -DskipTests. Additionally, line 123 shows mvn clean install -DskipTests test, which contradicts itself by skipping tests and then attempting to run them. Update both sections to accurately reflect Maven's lifecycle behavior.
Also applies to: 117-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/README-EXTERNAL-TESTING.md`
around lines 73 - 77, Update the README wording to correctly state that Maven
runs tests by default and that mvn clean install includes the test phase (so
replace the incorrect line claiming mvn clean install does not run tests); also
fix the contradictory example that shows mvn clean install -DskipTests test by
either removing -DskipTests or changing the command to a valid combination
(e.g., mvn clean install to run tests, or mvn clean install -DskipTests to skip
them, or use mvn test/mvn clean test where appropriate); ensure the README
examples and explanations consistently state that tests are run by default
unless -DskipTests is provided and adjust the two affected examples/lines
accordingly.
| ### Error: Server Extension Still Attempting Auto-Start | ||
| **Problem**: Tests fail with connection refused errors | ||
| ``` | ||
| java.net.ConnectException: Connection refused | ||
| ``` | ||
|
|
||
| **Solution**: | ||
| 1. Verify external IS is running on configured host/port | ||
| 2. Check firewall allows connection to IS port | ||
| 3. Verify ports are not changed in `-D` parameters | ||
|
|
||
| ### Error: Server Extension Still Attempting Auto-Start | ||
| **Problem**: Tests still try to extract pack from `modules/distribution/target/` |
There was a problem hiding this comment.
Use distinct headings for different troubleshooting cases.
The heading “Error: Server Extension Still Attempting Auto-Start” appears twice (Line 126 and Line 137). Rename one heading to match its specific failure mode.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 137-137: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/README-EXTERNAL-TESTING.md`
around lines 126 - 138, The README contains two identical headings "Error:
Server Extension Still Attempting Auto-Start"; change the second heading (the
one above the note about tests extracting pack from
`modules/distribution/target/`) to a distinct, descriptive heading such as
"Error: Tests Extracting Pack from modules/distribution/target" so each
troubleshooting case has a unique title; update the heading text only (keep
surrounding content) to reflect the specific failure mode and ensure clarity.
2 participants
Make backend integration test endpoints configurable
Terminal 1: Start your external IS pack manually
cd /path/to/external/wso2is/bin
./wso2server.sh
Terminal 2: Run tests against it (using automation-external.xml)
mvn -pl modules/integration/tests-integration/tests-backend test
-Dautomation.config.file=automation-external.xml
-Dintegration.test.is.host=localhost
-Dintegration.test.is.https.port=9853