Add tests for remote logging#14230
Open
thivindu wants to merge 1 commit into
Open
Conversation
thivindu
requested review from
AnuGayan,
Arshardh,
chamilaadhi and
tharindu1st
as code owners
Contributor
📝 WalkthroughWalkthroughAdds a new remote logging admin client, Log4j2 fixtures, and integration tests covering appender updates, sync behavior, deduplication, and HTTP delivery. It also updates TestNG wiring and the remote logging stub dependency version. ChangesRemote Logging Appender Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java (4)
887-889: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider replacing fixed sleep with polling for actual config file change.
waitForLog4j2ConfigSyncusesThread.sleep(seconds)which is a fixed delay. While the comment explains this is for OSGi reconfiguration, the test could be more resilient by polling the actual file content or a JMX/health indicator until the expected change is visible, with a timeout. This would reduce test flakiness and runtime when the sync is faster than the fixed delay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java` around lines 887 - 889, `waitForLog4j2ConfigSync` currently uses a fixed sleep, which makes `RemoteLoggingAppenderTest` slower and flaky; replace the `TimeUnit.SECONDS.sleep` delay with a polling-based wait that repeatedly checks the actual Log4j2 config state or file contents until the expected change is observed or a timeout is reached. Keep the helper method name the same so callers remain unchanged, but update its implementation to use a bounded retry loop with a small interval and clear timeout handling.
652-663: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnable or remove commented-out logger wiring assertions.
Lines 652-663 contain commented-out assertions that verify
AUDIT_LOGandAPI_LOGappear in theloggerslist with correctappenderRefvalues, and thatrootLoggerreferencesCARBON_LOGFILE. These are important completeness checks for the "all three missing" scenario. Either enable them or remove with explanation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java` around lines 652 - 663, The RemoteLoggingAppenderTest contains commented-out assertions for the all-three-missing scenario, so the completeness checks for AUDIT_LOG, API_LOG, and rootLogger wiring are currently skipped. In the relevant test method in RemoteLoggingAppenderTest, either re-enable the assertions that call isLoggerInList and hasLoggerAppenderRef for LOGGER_KEY_AUDIT, LOGGER_KEY_API, and the root logger appender ref, or remove them only if this case is intentionally no longer covered. Keep the assertions aligned with the existing LOGGER_KEY_AUDIT, LOGGER_KEY_API, APPENDER_AUDIT, APPENDER_API, and APPENDER_CARBON symbols so the test still verifies the expected logger wiring.
472-475: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the server-side NPE as a known issue.
The comment at lines 472-475 describes a
NullPointerExceptioninisDataUpdatedon the server side whengetUsername()returns null. This is a genuine server-side bug that the test works around. Consider filing a bug ticket or adding a TODO comment with a reference so the workaround can be removed once the server code is fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java` around lines 472 - 475, The comment in RemoteLoggingAppenderTest should explicitly mark the server-side NullPointerException in isDataUpdated as a known issue rather than just describing the workaround. Update the nearby test fixture note that references getRemoteServerConfigs and getUsername().equals(...) to include a TODO or bug reference tied to isDataUpdated so the workaround is clearly tracked and can be removed once the server-side fix lands.
451-454: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnable or remove commented-out logger assertions.
The commented-out assertions at lines 451-454 verify that
API_LOGappears in theloggerslist and has anappenderReftoAPI_LOGFILEafter remote logging is enabled. These checks are important for ensuring the logger wiring is correct when a missing appender block is created. Either enable them if they pass, or remove them with a comment explaining why they are not valid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java` around lines 451 - 454, The RemoteLoggingAppenderTest is leaving important logger verification disabled by commenting out assertions in the API remote logging check. Update the test around the logger validation logic to either re-enable the assertions that use isLoggerInList and hasLoggerAppenderRef for LOGGER_KEY_API and APPENDER_API, or remove them and add a brief justification comment if the behavior is no longer valid. Keep the test consistent with the rest of the remote logging assertions so the API logger wiring is still verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@all-in-one-apim/modules/integration/tests-common/admin-clients/src/main/java/org/wso2/am/admin/clients/logging/RemoteLoggingConfigClient.java`:
- Around line 94-107: The admin mutation call in
RemoteLoggingConfigClient.sendConfigOp is using fireAndForget, which can swallow
SOAP faults and defer failures. Replace the
stub._getServiceClient().fireAndForget(op) call with sendRobust for this request
path so add/reset errors surface immediately at the admin call. Keep the
existing request-building logic in sendConfigOp and preserve the op element
construction, only changing the transport invocation.
---
Nitpick comments:
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java`:
- Around line 887-889: `waitForLog4j2ConfigSync` currently uses a fixed sleep,
which makes `RemoteLoggingAppenderTest` slower and flaky; replace the
`TimeUnit.SECONDS.sleep` delay with a polling-based wait that repeatedly checks
the actual Log4j2 config state or file contents until the expected change is
observed or a timeout is reached. Keep the helper method name the same so
callers remain unchanged, but update its implementation to use a bounded retry
loop with a small interval and clear timeout handling.
- Around line 652-663: The RemoteLoggingAppenderTest contains commented-out
assertions for the all-three-missing scenario, so the completeness checks for
AUDIT_LOG, API_LOG, and rootLogger wiring are currently skipped. In the relevant
test method in RemoteLoggingAppenderTest, either re-enable the assertions that
call isLoggerInList and hasLoggerAppenderRef for LOGGER_KEY_AUDIT,
LOGGER_KEY_API, and the root logger appender ref, or remove them only if this
case is intentionally no longer covered. Keep the assertions aligned with the
existing LOGGER_KEY_AUDIT, LOGGER_KEY_API, APPENDER_AUDIT, APPENDER_API, and
APPENDER_CARBON symbols so the test still verifies the expected logger wiring.
- Around line 472-475: The comment in RemoteLoggingAppenderTest should
explicitly mark the server-side NullPointerException in isDataUpdated as a known
issue rather than just describing the workaround. Update the nearby test fixture
note that references getRemoteServerConfigs and getUsername().equals(...) to
include a TODO or bug reference tied to isDataUpdated so the workaround is
clearly tracked and can be removed once the server-side fix lands.
- Around line 451-454: The RemoteLoggingAppenderTest is leaving important logger
verification disabled by commenting out assertions in the API remote logging
check. Update the test around the logger validation logic to either re-enable
the assertions that use isLoggerInList and hasLoggerAppenderRef for
LOGGER_KEY_API and APPENDER_API, or remove them and add a brief justification
comment if the behavior is no longer valid. Keep the test consistent with the
rest of the remote logging assertions so the API logger wiring is still
verified.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8895b3f-2fcc-4a49-ae00-a8dccfdb067c
📒 Files selected for processing (10)
all-in-one-apim/modules/integration/tests-common/admin-clients/pom.xmlall-in-one-apim/modules/integration/tests-common/admin-clients/src/main/java/org/wso2/am/admin/clients/logging/RemoteLoggingConfigClient.javaall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.javaall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithAllAppenders.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutApiAppender.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutAuditAppender.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutCarbonAppender.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutLocalAppenders.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/testng.xmlall-in-one-apim/pom.xml
thivindu
force-pushed
the
remote-logging-tests
branch
from
297b330 to
22aa768
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java`:
- Around line 673-677: The cleanup in restoreAfterMethod() and the related
teardown path for remote logging state should not only log and continue when
reset fails. Update the RemoteLoggingAppenderTest cleanup flow around
disableAllActiveRemoteConfigs() and restoreLog4j2Properties() so any failure to
restore the baseline is surfaced as a test failure, using the existing teardown
methods to throw or assert after logging the error. Apply the same behavior to
the other cleanup block referenced in this test class so shared server state
cannot be left partially modified without failing the suite.
- Around line 4-8: The Apache license header in RemoteLoggingAppenderTest
contains a malformed URL, so update the license comment in the test file to use
the standard Apache 2.0 link and remove the stray trailing text from the URL.
Keep the rest of the header unchanged and make sure the corrected link is the
canonical http://www.apache.org/licenses/LICENSE-2.0 reference.
- Around line 574-598: The await in RemoteLoggingAppenderTest is only checking
that receivedLogPayloads is non-empty, which can pass because of earlier traffic
or addRemoteServerConfig() activity. Update triggerAuditLogEntry()/the wait
block to capture the payload count before triggering the admin action and then
wait for the count to increase, or otherwise assert a newly received payload
from the AUDIT appender rather than any existing entry. Make the assertion in
the atMost/poll loop verify a new log delivery was added to receivedLogPayloads.
- Around line 131-132: The mock logging server setup in
RemoteLoggingAppenderTest is using findFreePort() before HttpServer.create(),
which can race with other processes and cause flaky binds. Update the test
initialization to create the server with a port-0 InetSocketAddress directly,
then read the assigned port from the HttpServer instance for mockServerPort;
remove the now-unneeded findFreePort() helper and its call sites, including the
one used in the test setup and the duplicate helper near the bottom of the
class.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e41fefcb-42b6-4a3f-a0f1-997d4bda3d80
📒 Files selected for processing (10)
all-in-one-apim/modules/integration/tests-common/admin-clients/pom.xmlall-in-one-apim/modules/integration/tests-common/admin-clients/src/main/java/org/wso2/am/admin/clients/logging/RemoteLoggingConfigClient.javaall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.javaall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithAllAppenders.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutApiAppender.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutAuditAppender.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutCarbonAppender.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutLocalAppenders.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/testng.xmlall-in-one-apim/pom.xml
🚧 Files skipped from review as they are similar to previous changes (9)
- all-in-one-apim/modules/integration/tests-common/admin-clients/pom.xml
- all-in-one-apim/pom.xml
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithAllAppenders.properties
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutLocalAppenders.properties
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutApiAppender.properties
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutAuditAppender.properties
- all-in-one-apim/modules/integration/tests-common/admin-clients/src/main/java/org/wso2/am/admin/clients/logging/RemoteLoggingConfigClient.java
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutCarbonAppender.properties
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/testng.xml
Comment on lines
+131
to
+132
| mockServerPort = findFreePort(); | ||
| mockLogServer = HttpServer.create(new InetSocketAddress(mockServerPort), 0); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java"
echo "== file stats =="
wc -l "$FILE"
echo
echo "== outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== relevant slices =="
sed -n '110,160p' "$FILE"
echo
sed -n '870,920p' "$FILE"
echo
sed -n '880,910p' "$FILE" | nl -ba
echo
echo "== helper usages =="
rg -n "findFreePort\(|mockServerPort|HttpServer.create" "$FILE"Repository: wso2/product-apim
Length of output: 6055
Bind the mock server directly on port 0. findFreePort() closes the socket before HttpServer binds, so another process can grab the port and make this test fail intermittently. Create the server with new InetSocketAddress(0), read back the assigned port, and remove findFreePort() (also at lines 894-898).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java`
around lines 131 - 132, The mock logging server setup in
RemoteLoggingAppenderTest is using findFreePort() before HttpServer.create(),
which can race with other processes and cause flaky binds. Update the test
initialization to create the server with a port-0 InetSocketAddress directly,
then read the assigned port from the HttpServer instance for mockServerPort;
remove the now-unneeded findFreePort() helper and its call sites, including the
one used in the test setup and the duplicate helper near the bottom of the
class.
Comment on lines
+574
to
+598
| RemoteServerLoggerData auditConfig = buildRemoteConfig(LOG_TYPE_AUDIT); | ||
| remoteLoggingConfigClient.addRemoteServerConfig(auditConfig); | ||
| waitForLog4j2ConfigSync(5); | ||
|
|
||
| /* Verify the appender was converted to HTTP type */ | ||
| String afterEnable = readServerLog4j2Properties(); | ||
| assertEquals(extractAppenderType(APPENDER_AUDIT, afterEnable), HTTP_TYPE, | ||
| "AUDIT_LOGFILE must be HTTP type after enabling remote logging"); | ||
| assertTrue(afterEnable.contains("http://localhost:" + mockServerPort + "/api/logs/consume"), | ||
| "Remote URL must be present in the AUDIT_LOGFILE block"); | ||
|
|
||
| /* | ||
| * Trigger an admin action that produces an audit log entry. | ||
| * The admin REST API is authenticated, so calling any admin resource with valid | ||
| * credentials generates an AUDIT_LOG entry. | ||
| */ | ||
| triggerAuditLogEntry(); | ||
|
|
||
| /* Wait for the log entry to arrive at the mock server (up to 30 seconds) */ | ||
| with().pollInterval(2, TimeUnit.SECONDS).await().atMost(30, TimeUnit.SECONDS).untilAsserted(() -> { | ||
| synchronized (receivedLogPayloads) { | ||
| assertFalse(receivedLogPayloads.isEmpty(), | ||
| "Mock log server must have received at least one log payload from the AUDIT appender"); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wait for a new payload, not just any payload.
receivedLogPayloads is class-scoped, and Line 595 only checks for non-empty. If addRemoteServerConfig() or prior traffic already populated the buffer, this await passes without proving that Line 590 produced a delivery.
Proposed fix
- triggerAuditLogEntry();
+ int payloadCountBeforeTrigger;
+ synchronized (receivedLogPayloads) {
+ payloadCountBeforeTrigger = receivedLogPayloads.size();
+ }
+ triggerAuditLogEntry();
/* Wait for the log entry to arrive at the mock server (up to 30 seconds) */
with().pollInterval(2, TimeUnit.SECONDS).await().atMost(30, TimeUnit.SECONDS).untilAsserted(() -> {
synchronized (receivedLogPayloads) {
- assertFalse(receivedLogPayloads.isEmpty(),
- "Mock log server must have received at least one log payload from the AUDIT appender");
+ assertTrue(receivedLogPayloads.size() > payloadCountBeforeTrigger,
+ "Mock log server must receive a new log payload from the AUDIT appender");
}
});📝 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
| RemoteServerLoggerData auditConfig = buildRemoteConfig(LOG_TYPE_AUDIT); | |
| remoteLoggingConfigClient.addRemoteServerConfig(auditConfig); | |
| waitForLog4j2ConfigSync(5); | |
| /* Verify the appender was converted to HTTP type */ | |
| String afterEnable = readServerLog4j2Properties(); | |
| assertEquals(extractAppenderType(APPENDER_AUDIT, afterEnable), HTTP_TYPE, | |
| "AUDIT_LOGFILE must be HTTP type after enabling remote logging"); | |
| assertTrue(afterEnable.contains("http://localhost:" + mockServerPort + "/api/logs/consume"), | |
| "Remote URL must be present in the AUDIT_LOGFILE block"); | |
| /* | |
| * Trigger an admin action that produces an audit log entry. | |
| * The admin REST API is authenticated, so calling any admin resource with valid | |
| * credentials generates an AUDIT_LOG entry. | |
| */ | |
| triggerAuditLogEntry(); | |
| /* Wait for the log entry to arrive at the mock server (up to 30 seconds) */ | |
| with().pollInterval(2, TimeUnit.SECONDS).await().atMost(30, TimeUnit.SECONDS).untilAsserted(() -> { | |
| synchronized (receivedLogPayloads) { | |
| assertFalse(receivedLogPayloads.isEmpty(), | |
| "Mock log server must have received at least one log payload from the AUDIT appender"); | |
| } | |
| }); | |
| int payloadCountBeforeTrigger; | |
| synchronized (receivedLogPayloads) { | |
| payloadCountBeforeTrigger = receivedLogPayloads.size(); | |
| } | |
| triggerAuditLogEntry(); | |
| /* Wait for the log entry to arrive at the mock server (up to 30 seconds) */ | |
| with().pollInterval(2, TimeUnit.SECONDS).await().atMost(30, TimeUnit.SECONDS).untilAsserted(() -> { | |
| synchronized (receivedLogPayloads) { | |
| assertTrue(receivedLogPayloads.size() > payloadCountBeforeTrigger, | |
| "Mock log server must receive a new log payload from the AUDIT appender"); | |
| } | |
| }); |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 581-581: Avoid building a URL host from untrusted input
Context: "http://localhost:" + mockServerPort
Note: [CWE-20] Improper Input Validation.
(tainted-url-host)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java`
around lines 574 - 598, The await in RemoteLoggingAppenderTest is only checking
that receivedLogPayloads is non-empty, which can pass because of earlier traffic
or addRemoteServerConfig() activity. Update triggerAuditLogEntry()/the wait
block to capture the payload count before triggering the admin action and then
wait for the count to increase, or otherwise assert a newly received payload
from the AUDIT appender rather than any existing entry. Make the assertion in
the atMost/poll loop verify a new log delivery was added to receivedLogPayloads.
Comment on lines
+673
to
+677
| @AfterMethod(alwaysRun = true) | ||
| public void restoreAfterMethod() { | ||
| disableAllActiveRemoteConfigs(); | ||
| restoreLog4j2Properties(); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail the test when cleanup cannot restore the baseline.
restoreAfterMethod() logs and continues even when resetting remote configs or restoring log4j2.properties fails. Since this class mutates shared server state, warning-only cleanup turns the suite into an order-dependent false-positive generator.
Also applies to: 697-730
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java`
around lines 673 - 677, The cleanup in restoreAfterMethod() and the related
teardown path for remote logging state should not only log and continue when
reset fails. Update the RemoteLoggingAppenderTest cleanup flow around
disableAllActiveRemoteConfigs() and restoreLog4j2Properties() so any failure to
restore the baseline is surfaced as a test failure, using the existing teardown
methods to throw or assert after logging the error. Apply the same behavior to
the other cleanup block referenced in this test class so shared server state
cannot be left partially modified without failing the suite.
thivindu
force-pushed
the
remote-logging-tests
branch
from
22aa768 to
9314c65
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java (1)
451-454: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRemove or re-enable the commented-out logger-wiring assertions and now-unused helpers. The disabled assertions in
testOnlyTargetedAppenderCreatedWhenOthersAbsent(Lines 451-454) andtestAllThreeMissingAppendersCreatedOnEnable(Lines 652-663) drop verification that the recreated appenders are actually wired into theloggerslist /appenderRefs — so these tests confirm the block exists but not that it routes logs. With them commented out,isLoggerInList(Line 825),hasLoggerAppenderRef(Line 845), andhasRootLoggerAppenderRef(Line 854) are dead code. Either restore the assertions (preferred, to validate routing) or delete both the commented blocks and the unused helpers.Want me to wire up the logger-list assertions or strip the dead helpers?
Also applies to: 652-663
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java` around lines 451 - 454, Re-enable or remove the commented logger-wiring checks in RemoteLoggingAppenderTest so the tests still validate that recreated appenders are actually attached to the logging configuration, not just present in the XML. In testOnlyTargetedAppenderCreatedWhenOthersAbsent and testAllThreeMissingAppendersCreatedOnEnable, restore the assertions that use isLoggerInList, hasLoggerAppenderRef, and hasRootLoggerAppenderRef if you want to keep routing verification; otherwise delete those commented assertions and remove the now-unused helper methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.java`:
- Around line 451-454: Re-enable or remove the commented logger-wiring checks in
RemoteLoggingAppenderTest so the tests still validate that recreated appenders
are actually attached to the logging configuration, not just present in the XML.
In testOnlyTargetedAppenderCreatedWhenOthersAbsent and
testAllThreeMissingAppendersCreatedOnEnable, restore the assertions that use
isLoggerInList, hasLoggerAppenderRef, and hasRootLoggerAppenderRef if you want
to keep routing verification; otherwise delete those commented assertions and
remove the now-unused helper methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb4df39d-2f48-4cd9-8c68-6989a18bd693
📒 Files selected for processing (10)
all-in-one-apim/modules/integration/tests-common/admin-clients/pom.xmlall-in-one-apim/modules/integration/tests-common/admin-clients/src/main/java/org/wso2/am/admin/clients/logging/RemoteLoggingConfigClient.javaall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/logging/RemoteLoggingAppenderTest.javaall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithAllAppenders.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutApiAppender.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutAuditAppender.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutCarbonAppender.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutLocalAppenders.propertiesall-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/testng.xmlall-in-one-apim/pom.xml
✅ Files skipped from review due to trivial changes (1)
- all-in-one-apim/modules/integration/tests-common/admin-clients/pom.xml
🚧 Files skipped from review as they are similar to previous changes (8)
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutApiAppender.properties
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/testng.xml
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithAllAppenders.properties
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutAuditAppender.properties
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutCarbonAppender.properties
- all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/resources/artifacts/AM/configFiles/remoteLogging/log4j2WithoutLocalAppenders.properties
- all-in-one-apim/pom.xml
- all-in-one-apim/modules/integration/tests-common/admin-clients/src/main/java/org/wso2/am/admin/clients/logging/RemoteLoggingConfigClient.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds integration tests for the remote logging appender management behaviour, covering how
log4j2.propertiesis modified (or deliberately left unchanged) when remote server logging is enabled or disabled via theRemoteLoggingConfigadmin service.Related to - wso2/api-manager#4925
Changes
New:
RemoteLoggingConfigClientA thin wrapper around
RemoteLoggingConfigStubthat exposes the five operations of theRemoteLoggingConfigAxis2 admin service —addRemoteServerConfig,resetRemoteServerConfig,getRemoteServerConfigs,getRemoteServerConfig, andsyncRemoteServerConfigs— in a form that integrates with the existing admin-client authentication pattern.New:
RemoteLoggingAppenderTestTen independent integration tests grouped into four scenarios. Each test writes a dedicated fixture to the live
log4j2.properties, calls the service under test, reads the file back, and asserts on its content.@AfterMethod(alwaysRun = true)resets all active remote logging configurations and restores the originallog4j2.propertiesafter every test (pass or fail), providing full isolation withoutdependsOnMethodschaining.A built-in Java
HttpServeracts as the mock remote log receiver for the end-to-end test; no additional test dependencies are introduced.testExistingAppendersPreservedOnResetWithBlankUrlresetRemoteServerConfigon already-RollingFileappenders is a no-optestOnlyConfiguredAppenderIsResettestMissingAuditAppenderCreatedOnEnableAUDIT_LOGFILEblock written as HTTP +AUDIT_LOGlogger wired when block was absenttestMissingCarbonAppenderCreatedOnEnableCARBON_LOGFILEblock written as HTTP +rootLoggerappenderRef added when block was absenttestMissingApiAppenderCreatedOnEnableAPI_LOGFILEblock written as HTTP +API_LOGlogger wired when block was absenttestOnlyTargetedAppenderCreatedWhenOthersAbsentAPI_LOGFILE;AUDIT_LOGFILEandCARBON_LOGFILEremain absenttestMissingAppendersNotAddedOnSyncWhenRemoteLoggingNotConfiguredsyncRemoteServerConfigs()with no configured remote URLs leaves a strippedlog4j2.propertiesunchangedtestNoDuplicateAppenderInListappenders = ...listtestRemoteLoggingEndToEndtestAllThreeMissingAppendersCreatedOnEnableNew: log4j2 test fixtures (5 files)
Each fixture is a self-consistent
log4j2.propertiesfile. Loggers that reference absent appenders are removed to preventUnable to locate appenderwarnings during the test run.log4j2WithAllAppenders.propertiesRollingFile; baseline for reset and no-duplicate testslog4j2WithoutAuditAppender.propertiesAUDIT_LOGFILEblock andAUDIT_LOGlogger absentlog4j2WithoutCarbonAppender.propertiesCARBON_LOGFILEblock and itsrootLoggerreference absentlog4j2WithoutApiAppender.propertiesAPI_LOGFILEblock andAPI_LOGlogger absentlog4j2WithoutLocalAppenders.propertiesrootLoggerreference absentModified:
pom.xmlfilespom.xml— addedorg.wso2.carbon.logging.remote.config.stubtodependencyManagementadmin-clients/pom.xml— consumed the above dependencyModified:
testng.xmlAdded the
apim-remote-logging-teststest group pointing atRemoteLoggingAppenderTest.Testing
The end-to-end test (
testRemoteLoggingEndToEnd) requires no external infrastructure — it binds an ephemeralHttpServeron a free local port and uses Awaitility to wait up to 30 seconds for the first log payload to arrive.