Refactor Example Tests and move examples/offline into /configuration/offline.apis.yaml#2833
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRemoved OS-specific offline launchers; introduced test-base extension points for passing CLI parameters and env vars; Process2.Builder API renamed to Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/OfflineExampleTest.java`:
- Around line 34-37: The startMembrane() method in OfflineExampleTest currently
overrides the lifecycle method from AbstractSampleMembraneStartStopTestcase but
lacks the `@Override` annotation; add `@Override` above the startMembrane()
declaration (keeping the existing `@BeforeEach` and throws signature) to make the
override explicit and safer during refactors, referencing the startMembrane()
method in OfflineExampleTest and the superclass
AbstractSampleMembraneStartStopTestcase.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
distribution/examples/configuration/offline.apis.yamldistribution/examples/offline/membrane.cmddistribution/examples/offline/membrane.shdistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/OfflineExampleTest.java
💤 Files with no reviewable changes (2)
- distribution/examples/offline/membrane.sh
- distribution/examples/offline/membrane.cmd
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationSimpleExampleTest.java (2)
27-51: Declare test-data fields asstatic finalconstants.These text-block literals are immutable; making them instance fields allocates them on every test instantiation without purpose.
♻️ Proposed refactor
- final String createPersonValid = """ + static final String createPersonValid = """ {"name": "Johannes Gutenberg","age": 78} """; - final String createPersonInvalid = """ + static final String createPersonInvalid = """ {"name": "Johannes Gutenberg","age": -10} """; - final String validationResult = """ + static final String validationResult = """ ... """;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationSimpleExampleTest.java` around lines 27 - 51, The string test fixtures createPersonValid, createPersonInvalid, and validationResult are immutable and should be declared as static final constants to avoid per-test-instance allocation; update their declarations in OpenAPIValidationSimpleExampleTest from instance fields to static final fields (e.g., change the declarations of createPersonValid, createPersonInvalid, and validationResult to static final).
58-83: Split the two scenarios into separate@Testmethods.Both scenarios are independent; bundling them in one method means a failure in the valid-path assertion silently prevents the invalid-path scenario from ever running, masking a second potential failure.
♻️ Proposed refactor
- `@Test` - public void test() throws Exception { - // `@formatter`:off - // Test valid person creation - given() - .contentType(JSON) - .body(createPersonValid) - .when() - .post(LOCALHOST_2000 + "/persons") - .then() - .statusCode(201) - .body(containsString("success")); - - // Test invalid person creation - String response =given() - .contentType(JSON) - .body(createPersonInvalid) - .when() - .post(LOCALHOST_2000 + "/persons") - .then() - .statusCode(400) - .extract().response().asString(); - // `@formatter`:on - - JSONAssert.assertEquals(validationResult, response, false); - } + `@Test` + public void validPersonCreationReturns201() { + // `@formatter`:off + given() + .contentType(JSON) + .body(createPersonValid) + .when() + .post(LOCALHOST_2000 + "/persons") + .then() + .statusCode(201) + .body(containsString("success")); + // `@formatter`:on + } + + `@Test` + public void invalidPersonCreationReturns400WithValidationDetails() throws Exception { + // `@formatter`:off + String response = given() + .contentType(JSON) + .body(createPersonInvalid) + .when() + .post(LOCALHOST_2000 + "/persons") + .then() + .statusCode(400) + .extract().response().asString(); + // `@formatter`:on + + JSONAssert.assertEquals(validationResult, response, false); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationSimpleExampleTest.java` around lines 58 - 83, The single test method test() mixes two independent scenarios; split it into two `@Test` methods (e.g., testCreatePersonValid and testCreatePersonInvalid) so the valid-path request using given().contentType(JSON).body(createPersonValid).when().post(...).then().statusCode(201).body(containsString("success")) runs separately from the invalid-path request that uses given().contentType(JSON).body(createPersonInvalid).when().post(...).then().statusCode(400).extract().response().asString() and JSONAssert.assertEquals(validationResult, response, false); move the JSONAssert call into the invalid test, preserve any shared setup as class fields or a `@Before` method, and keep all request/response plumbing identical to the original to avoid changing behavior.distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/SOAPProxyValidationExampleTest.java (1)
34-44: Add.log().ifValidationFails()for diagnostic consistency.
testInvalidCitySoapRequest()logs on failure (line 56) buttestValidCitySoapRequest()does not. When the valid-request test fails in CI, there's no response body logged, making failures harder to diagnose.♻️ Proposed fix
.then() + .log().ifValidationFails() .body(not(containsString("faultcode"))) .statusCode(200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/SOAPProxyValidationExampleTest.java` around lines 34 - 44, The testValidCitySoapRequest method in SOAPProxyValidationExampleTest should mirror the diagnostic logging used in testInvalidCitySoapRequest; update the RestAssured request chain within testValidCitySoapRequest to include .log().ifValidationFails() before the assertions so the response body is logged on failures (add the call in the given()/.when() chain surrounding the existing contentType(...).body(readFile("city-soap.xml")).post(...) sequence).distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/CBRXPathExampleTest.java (1)
34-55: Make XMLContent-Typeexplicit for all three POSTs.At Lines 34-55, two XML requests (
order.xml,import.xml) omit.contentType(APPLICATION_XML)whileexpress.xmlsets it. REST Assured does not automatically add aContent-Typeheader when.contentType()is not called, so making it explicit ensures consistency and clearly expresses test intent rather than relying on implicit server behavior.Suggested patch
given() + .contentType(APPLICATION_XML) .body(readFile("order.xml")) .post(LOCALHOST_2000) .then() @@ given() + .contentType(APPLICATION_XML) .body(readFile("import.xml")) .post(LOCALHOST_2000) .then()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/CBRXPathExampleTest.java` around lines 34 - 55, Two of the REST Assured requests use given().body(readFile(...)) without setting Content-Type while express.xml does; update the first and third POST requests (the ones calling readFile("order.xml") and readFile("import.xml") in CBRXPathExampleTest) to include .contentType(APPLICATION_XML) before .body(...) or before .post(LOCALHOST_2000) so all three XML requests explicitly set the XML Content-Type header for consistency and clarity.distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/InternalProxyExampleTest.java (1)
51-52: Unused.extract().asString()return valuesThe extracted strings in both the second and third
given/when/thenchains are discarded. Unless body-inspection logic is planned, drop the.extract().asString()tails to keep the assertions concise.♻️ Proposed cleanup
.then() .statusCode(200) - .body(containsStringIgnoringCase("Normal")) - .extract() - .asString(); + .body(containsStringIgnoringCase("Normal"));Apply the same change to lines 67-69.
Also applies to: 68-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/InternalProxyExampleTest.java` around lines 51 - 52, In InternalProxyExampleTest remove the unused .extract().asString() calls from the second and third given/when/then chains (they discard the return value) so the assertions read directly from the response; locate the chains in class InternalProxyExampleTest and delete the .extract().asString() tail occurrences (also remove the duplicate at the other occurrence noted around the same test block) to keep assertions concise.distribution/src/test/java/com/predic8/membrane/examples/util/Process2.java (1)
85-93: Add null-safety at the new Builder API boundary.
withEnv(null)andwithParameters(null)can produce brittle runtime behavior. Guard these inputs early to avoid opaque failures later in startup.♻️ Proposed hardening
public Builder withEnv(Map<String, String> envs) { - env.putAll(envs); + if (envs == null || envs.isEmpty()) + return this; + env.putAll(envs); return this; } public Builder withParameters(String parameters) { - this.parameters = parameters; + this.parameters = parameters == null ? "" : parameters; return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/examples/util/Process2.java` around lines 85 - 93, The Builder API should guard against null inputs: update Builder.withEnv(Map<String,String> envs) to check for null before calling env.putAll (e.g., if envs != null then env.putAll(envs)) so withEnv(null) becomes a no-op, and update Builder.withParameters(String parameters) to null-safe assignment (e.g., this.parameters = parameters != null ? parameters : "" or another neutral default) so withParameters(null) won’t produce NPEs later; keep using the existing env field and parameters field names and the same method signatures.distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/ThrottleExampleTest.java (1)
34-36: Use a monotonic clock for elapsed-time assertion.At lines 34 and 36,
System.currentTimeMillis()can shift with system clock adjustments, producing flaky results in timing assertions. Replace withSystem.nanoTime(), which is monotonic and expressly designed for duration measurement.Proposed fix
+import java.time.Duration; @@ - long start = System.currentTimeMillis(); + long start = System.nanoTime(); when().get(LOCALHOST_2000).then().statusCode(200); - assertTrue(System.currentTimeMillis() - start >= 1000); + assertTrue(System.nanoTime() - start >= Duration.ofSeconds(1).toNanos());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/ThrottleExampleTest.java` around lines 34 - 36, In ThrottleExampleTest replace the use of System.currentTimeMillis() for measuring elapsed time with System.nanoTime(): capture start = System.nanoTime() before the request and compute the elapsed duration using System.nanoTime() - start, then assert against an equivalent nanosecond threshold (e.g. >= 1_000_000_000L) or convert to milliseconds by dividing by 1_000_000 before comparing to 1000; update the assertion that currently references start and System.currentTimeMillis() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/AccessLogExampleTest.java`:
- Around line 34-47: The test currently calls WaitableConsoleEvent.occurred()
which can return false if log output is delayed; update the assertion to use
WaitableConsoleEvent.waitFor(...) with a bounded timeout (e.g. a few seconds) to
wait for the event instead: locate the console variable in the console() test
method and replace the assertTrue(console.occurred()) call with an assertion
that checks the boolean result of console.waitFor(timeoutMillis) (use
WaitableConsoleEvent.waitFor) so the test waits deterministically for the log
event.
---
Nitpick comments:
In `@distribution/src/test/java/com/predic8/membrane/examples/util/Process2.java`:
- Around line 85-93: The Builder API should guard against null inputs: update
Builder.withEnv(Map<String,String> envs) to check for null before calling
env.putAll (e.g., if envs != null then env.putAll(envs)) so withEnv(null)
becomes a no-op, and update Builder.withParameters(String parameters) to
null-safe assignment (e.g., this.parameters = parameters != null ? parameters :
"" or another neutral default) so withParameters(null) won’t produce NPEs later;
keep using the existing env field and parameters field names and the same method
signatures.
In
`@distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/ThrottleExampleTest.java`:
- Around line 34-36: In ThrottleExampleTest replace the use of
System.currentTimeMillis() for measuring elapsed time with System.nanoTime():
capture start = System.nanoTime() before the request and compute the elapsed
duration using System.nanoTime() - start, then assert against an equivalent
nanosecond threshold (e.g. >= 1_000_000_000L) or convert to milliseconds by
dividing by 1_000_000 before comparing to 1000; update the assertion that
currently references start and System.currentTimeMillis() accordingly.
In
`@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationSimpleExampleTest.java`:
- Around line 27-51: The string test fixtures createPersonValid,
createPersonInvalid, and validationResult are immutable and should be declared
as static final constants to avoid per-test-instance allocation; update their
declarations in OpenAPIValidationSimpleExampleTest from instance fields to
static final fields (e.g., change the declarations of createPersonValid,
createPersonInvalid, and validationResult to static final).
- Around line 58-83: The single test method test() mixes two independent
scenarios; split it into two `@Test` methods (e.g., testCreatePersonValid and
testCreatePersonInvalid) so the valid-path request using
given().contentType(JSON).body(createPersonValid).when().post(...).then().statusCode(201).body(containsString("success"))
runs separately from the invalid-path request that uses
given().contentType(JSON).body(createPersonInvalid).when().post(...).then().statusCode(400).extract().response().asString()
and JSONAssert.assertEquals(validationResult, response, false); move the
JSONAssert call into the invalid test, preserve any shared setup as class fields
or a `@Before` method, and keep all request/response plumbing identical to the
original to avoid changing behavior.
In
`@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/CBRXPathExampleTest.java`:
- Around line 34-55: Two of the REST Assured requests use
given().body(readFile(...)) without setting Content-Type while express.xml does;
update the first and third POST requests (the ones calling readFile("order.xml")
and readFile("import.xml") in CBRXPathExampleTest) to include
.contentType(APPLICATION_XML) before .body(...) or before .post(LOCALHOST_2000)
so all three XML requests explicitly set the XML Content-Type header for
consistency and clarity.
In
`@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/InternalProxyExampleTest.java`:
- Around line 51-52: In InternalProxyExampleTest remove the unused
.extract().asString() calls from the second and third given/when/then chains
(they discard the return value) so the assertions read directly from the
response; locate the chains in class InternalProxyExampleTest and delete the
.extract().asString() tail occurrences (also remove the duplicate at the other
occurrence noted around the same test block) to keep assertions concise.
In
`@distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/SOAPProxyValidationExampleTest.java`:
- Around line 34-44: The testValidCitySoapRequest method in
SOAPProxyValidationExampleTest should mirror the diagnostic logging used in
testInvalidCitySoapRequest; update the RestAssured request chain within
testValidCitySoapRequest to include .log().ifValidationFails() before the
assertions so the response body is logged on failures (add the call in the
given()/.when() chain surrounding the existing
contentType(...).body(readFile("city-soap.xml")).post(...) sequence).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
distribution/src/test/java/com/predic8/membrane/examples/util/DistributionExtractingTestcase.javadistribution/src/test/java/com/predic8/membrane/examples/util/Process2.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/DistributionApisYamlExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/config/ProxiesXMLExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/rest2soap/Rest2SOAPTemplateExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/ssl/ToBackendExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/ACLExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/ConfigurationPropertiesTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/FileExchangeStoreExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/FormValidationExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/Loadbalancing4XmlSessionExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/LoggingCSVExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/LoggingJsonExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/OutgoingAPIGatewayExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/RewriterExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/SpELExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/ThrottleExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/VersioningSoapXsltExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/OfflineExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/message_transformation/Xml2JsonExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationSimpleExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/AccessLogExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/CBRXPathExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/InternalProxyExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/Loadbalancing6HealthMonitorExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/OAuth2APIExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/OAuth2CredentialsExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/XMLValidationExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.javadistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/SOAPProxyValidationExampleTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/advanced/EnvironmentVariablesTutorialTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/OfflineExampleTest.java
… process management.
predic8
approved these changes
Feb 25, 2026
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
2 participants
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 by CodeRabbit