Copilot commented on code in PR #4135:
URL:
https://github.com/apache/incubator-kie-kogito-runtimes/pull/4135#discussion_r2567598170
##########
quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-integration-test/src/test/java/org/kie/kogito/quarkus/workflows/OpenAPIInterfaceGenIT.java:
##########
@@ -51,4 +59,63 @@ void verifyOperationIdIsGeneratedByDefault() {
.body("paths.'/helloworld'.get.operationId",
is("getAllProcessInstances_helloworld"));
}
+ @ParameterizedTest
+ @ValueSource(strings = { "helloworld", "expression", "greet", "long-call",
"squareService" })
+ void verifyProcessPrefixSchemasForMultipleWorkflows(String processId) {
+ String openapi =
+ RestAssured.given()
+ .accept(ContentType.JSON)
+ .when()
+ .get("/q/openapi?format=json")
+ .then()
+ .statusCode(200)
+ .extract()
+ .asString();
+
+ JsonPath jp = new JsonPath(openapi);
+
+ Map<String, Object> schemas = jp.getMap("components.schemas");
+ assertTrue(schemas.containsKey("Expression Input"));
+ assertTrue(schemas.containsKey("Expression Output"));
+
+ Map<String, Object> inputSchema =
jp.getMap("components.schemas['Expression Input']");
+ assertNotNull(inputSchema, "Input schema should exist for processId: "
+ processId);
+ assertTrue(inputSchema.containsKey("type"), "Input schema should
contain 'type' field for processId: " + processId);
+
+ List<String> postTags = jp.getList("paths.'/" + processId +
"'.post.tags");
+ assertTrue(postTags.contains("Process - " + processId));
+
+ Map<String, Object> getProps = jp.getMap("paths.'/" + processId +
"'.get.responses.'200'.content.'application/json'.schema.properties");
+ if (getProps != null) {
+ assertTrue(getProps.containsKey("workflowdata") ||
getProps.containsKey("id"));
+ }
Review Comment:
The conditional assertion `if (getProps != null)` on line 89 makes the test
pass even when `getProps` is null, which means the test doesn't fail when
expected properties are missing. This weakens test coverage. Consider using
`assertNotNull(getProps)` before checking its contents, or provide a clear
assertion message explaining when null is acceptable.
##########
quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-integration-test/src/test/java/org/kie/kogito/quarkus/workflows/OpenAPIInterfaceGenIT.java:
##########
@@ -51,4 +59,63 @@ void verifyOperationIdIsGeneratedByDefault() {
.body("paths.'/helloworld'.get.operationId",
is("getAllProcessInstances_helloworld"));
}
+ @ParameterizedTest
+ @ValueSource(strings = { "helloworld", "expression", "greet", "long-call",
"squareService" })
+ void verifyProcessPrefixSchemasForMultipleWorkflows(String processId) {
+ String openapi =
+ RestAssured.given()
+ .accept(ContentType.JSON)
+ .when()
+ .get("/q/openapi?format=json")
+ .then()
+ .statusCode(200)
+ .extract()
+ .asString();
+
+ JsonPath jp = new JsonPath(openapi);
+
+ Map<String, Object> schemas = jp.getMap("components.schemas");
+ assertTrue(schemas.containsKey("Expression Input"));
+ assertTrue(schemas.containsKey("Expression Output"));
+
+ Map<String, Object> inputSchema =
jp.getMap("components.schemas['Expression Input']");
+ assertNotNull(inputSchema, "Input schema should exist for processId: "
+ processId);
+ assertTrue(inputSchema.containsKey("type"), "Input schema should
contain 'type' field for processId: " + processId);
+
+ List<String> postTags = jp.getList("paths.'/" + processId +
"'.post.tags");
+ assertTrue(postTags.contains("Process - " + processId));
+
+ Map<String, Object> getProps = jp.getMap("paths.'/" + processId +
"'.get.responses.'200'.content.'application/json'.schema.properties");
+ if (getProps != null) {
+ assertTrue(getProps.containsKey("workflowdata") ||
getProps.containsKey("id"));
+ }
+ }
+
+ @Test
+ void verifyOperationsWithoutProcessPrefixAreNotModified() {
+ String openapi =
+ RestAssured.given()
+ .accept(ContentType.JSON)
+ .when()
+ .get("/q/openapi?format=json")
+ .then()
+ .statusCode(200)
+ .extract()
+ .asString();
+
+ JsonPath jp = new JsonPath(openapi);
+
+ List<String> tags = jp.getList("paths.'/noprefix'.post.tags");
+ if (tags != null) {
+ boolean hasProcessPrefix = tags.stream().anyMatch(tag ->
tag.startsWith("Process - "));
+ assertTrue(!hasProcessPrefix);
+ }
+
+ Map<String, Object> inProps =
jp.getMap("paths.'/noprefix'.post.requestBody.content.'application/json'.schema.properties");
+ assertTrue(inProps == null || inProps.isEmpty());
+
+ Map<String, Object> outProps =
jp.getMap("paths.'/noprefix'.get.responses.'200'.content.'application/json'.schema.properties");
+ assertTrue(outProps == null || outProps.isEmpty());
Review Comment:
The conditional checks on lines 109 and 114-118 allow the test to pass when
`tags`, `inProps`, or `outProps` are null. This weakens the test's ability to
catch regressions. If these properties are expected to be absent for the
'/noprefix' endpoint, add explicit assertions with descriptive messages.
Otherwise, assert they are not null before checking their values.
##########
quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-integration-test/src/test/java/org/kie/kogito/quarkus/workflows/OpenAPIInterfaceGenIT.java:
##########
@@ -51,4 +59,63 @@ void verifyOperationIdIsGeneratedByDefault() {
.body("paths.'/helloworld'.get.operationId",
is("getAllProcessInstances_helloworld"));
}
+ @ParameterizedTest
+ @ValueSource(strings = { "helloworld", "expression", "greet", "long-call",
"squareService" })
+ void verifyProcessPrefixSchemasForMultipleWorkflows(String processId) {
+ String openapi =
+ RestAssured.given()
+ .accept(ContentType.JSON)
+ .when()
+ .get("/q/openapi?format=json")
+ .then()
+ .statusCode(200)
+ .extract()
+ .asString();
+
+ JsonPath jp = new JsonPath(openapi);
+
+ Map<String, Object> schemas = jp.getMap("components.schemas");
+ assertTrue(schemas.containsKey("Expression Input"));
+ assertTrue(schemas.containsKey("Expression Output"));
+
+ Map<String, Object> inputSchema =
jp.getMap("components.schemas['Expression Input']");
+ assertNotNull(inputSchema, "Input schema should exist for processId: "
+ processId);
+ assertTrue(inputSchema.containsKey("type"), "Input schema should
contain 'type' field for processId: " + processId);
+
+ List<String> postTags = jp.getList("paths.'/" + processId +
"'.post.tags");
+ assertTrue(postTags.contains("Process - " + processId));
+
+ Map<String, Object> getProps = jp.getMap("paths.'/" + processId +
"'.get.responses.'200'.content.'application/json'.schema.properties");
+ if (getProps != null) {
+ assertTrue(getProps.containsKey("workflowdata") ||
getProps.containsKey("id"));
+ }
+ }
+
+ @Test
+ void verifyOperationsWithoutProcessPrefixAreNotModified() {
+ String openapi =
+ RestAssured.given()
+ .accept(ContentType.JSON)
+ .when()
+ .get("/q/openapi?format=json")
+ .then()
+ .statusCode(200)
+ .extract()
+ .asString();
Review Comment:
Similar to the previous test, this test also makes redundant HTTP calls.
Consider extracting the OpenAPI spec fetching to a shared setup method to avoid
duplicate code and improve test performance.
##########
quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-integration-test/src/test/java/org/kie/kogito/quarkus/workflows/OpenAPIInterfaceGenIT.java:
##########
@@ -51,4 +59,63 @@ void verifyOperationIdIsGeneratedByDefault() {
.body("paths.'/helloworld'.get.operationId",
is("getAllProcessInstances_helloworld"));
}
+ @ParameterizedTest
+ @ValueSource(strings = { "helloworld", "expression", "greet", "long-call",
"squareService" })
+ void verifyProcessPrefixSchemasForMultipleWorkflows(String processId) {
+ String openapi =
+ RestAssured.given()
+ .accept(ContentType.JSON)
+ .when()
+ .get("/q/openapi?format=json")
+ .then()
+ .statusCode(200)
+ .extract()
+ .asString();
Review Comment:
The test fetches the entire OpenAPI spec for each iteration of the
parameterized test (5 times total), which is inefficient. Consider using
`@BeforeAll` to fetch the OpenAPI spec once and store it in a static field,
then reuse it across all test iterations to improve test performance.
##########
quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-integration-test/src/test/java/org/kie/kogito/quarkus/workflows/OpenAPIInterfaceGenIT.java:
##########
@@ -51,4 +59,63 @@ void verifyOperationIdIsGeneratedByDefault() {
.body("paths.'/helloworld'.get.operationId",
is("getAllProcessInstances_helloworld"));
}
+ @ParameterizedTest
+ @ValueSource(strings = { "helloworld", "expression", "greet", "long-call",
"squareService" })
+ void verifyProcessPrefixSchemasForMultipleWorkflows(String processId) {
+ String openapi =
+ RestAssured.given()
+ .accept(ContentType.JSON)
+ .when()
+ .get("/q/openapi?format=json")
+ .then()
+ .statusCode(200)
+ .extract()
+ .asString();
+
+ JsonPath jp = new JsonPath(openapi);
+
+ Map<String, Object> schemas = jp.getMap("components.schemas");
+ assertTrue(schemas.containsKey("Expression Input"));
+ assertTrue(schemas.containsKey("Expression Output"));
+
+ Map<String, Object> inputSchema =
jp.getMap("components.schemas['Expression Input']");
Review Comment:
The test validates hardcoded "Expression Input" and "Expression Output"
schemas on lines 78-79, but the parameterized test iterates over multiple
process IDs including "helloworld", "greet", "long-call", and "squareService".
These hardcoded assertions will pass or fail regardless of which `processId`
parameter is being tested, making the parameterization misleading. Either
remove these hardcoded assertions or make them dependent on the `processId`
parameter to properly test each workflow's schemas.
```suggestion
String inputSchemaName = processId + " Input";
String outputSchemaName = processId + " Output";
assertTrue(schemas.containsKey(inputSchemaName), "Schemas should
contain input schema for processId: " + processId);
assertTrue(schemas.containsKey(outputSchemaName), "Schemas should
contain output schema for processId: " + processId);
Map<String, Object> inputSchema = jp.getMap("components.schemas['" +
inputSchemaName + "']");
```
##########
quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-integration-test/src/test/java/org/kie/kogito/quarkus/workflows/OpenAPIInterfaceGenIT.java:
##########
@@ -51,4 +59,63 @@ void verifyOperationIdIsGeneratedByDefault() {
.body("paths.'/helloworld'.get.operationId",
is("getAllProcessInstances_helloworld"));
}
+ @ParameterizedTest
+ @ValueSource(strings = { "helloworld", "expression", "greet", "long-call",
"squareService" })
+ void verifyProcessPrefixSchemasForMultipleWorkflows(String processId) {
Review Comment:
The test name `verifyProcessPrefixSchemasForMultipleWorkflows` doesn't
accurately describe what the test is validating. The test checks for
"Expression Input" and "Expression Output" schemas specifically, validates
tags, and checks response properties, but the name suggests it's verifying
process prefix schemas generically for all workflows. Consider renaming to
something more specific like `verifySchemaAndTagsForWorkflows` or split this
into separate focused tests.
```suggestion
void verifySchemaAndTagsForWorkflows(String processId) {
```
##########
quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-integration-test/src/test/java/org/kie/kogito/quarkus/workflows/OpenAPIInterfaceGenIT.java:
##########
@@ -51,4 +59,63 @@ void verifyOperationIdIsGeneratedByDefault() {
.body("paths.'/helloworld'.get.operationId",
is("getAllProcessInstances_helloworld"));
}
+ @ParameterizedTest
+ @ValueSource(strings = { "helloworld", "expression", "greet", "long-call",
"squareService" })
+ void verifyProcessPrefixSchemasForMultipleWorkflows(String processId) {
+ String openapi =
+ RestAssured.given()
+ .accept(ContentType.JSON)
+ .when()
+ .get("/q/openapi?format=json")
+ .then()
+ .statusCode(200)
+ .extract()
+ .asString();
+
+ JsonPath jp = new JsonPath(openapi);
+
+ Map<String, Object> schemas = jp.getMap("components.schemas");
+ assertTrue(schemas.containsKey("Expression Input"));
+ assertTrue(schemas.containsKey("Expression Output"));
+
+ Map<String, Object> inputSchema =
jp.getMap("components.schemas['Expression Input']");
+ assertNotNull(inputSchema, "Input schema should exist for processId: "
+ processId);
+ assertTrue(inputSchema.containsKey("type"), "Input schema should
contain 'type' field for processId: " + processId);
+
+ List<String> postTags = jp.getList("paths.'/" + processId +
"'.post.tags");
+ assertTrue(postTags.contains("Process - " + processId));
+
+ Map<String, Object> getProps = jp.getMap("paths.'/" + processId +
"'.get.responses.'200'.content.'application/json'.schema.properties");
+ if (getProps != null) {
+ assertTrue(getProps.containsKey("workflowdata") ||
getProps.containsKey("id"));
+ }
+ }
+
+ @Test
+ void verifyOperationsWithoutProcessPrefixAreNotModified() {
+ String openapi =
+ RestAssured.given()
+ .accept(ContentType.JSON)
+ .when()
+ .get("/q/openapi?format=json")
+ .then()
+ .statusCode(200)
+ .extract()
+ .asString();
+
+ JsonPath jp = new JsonPath(openapi);
+
+ List<String> tags = jp.getList("paths.'/noprefix'.post.tags");
+ if (tags != null) {
+ boolean hasProcessPrefix = tags.stream().anyMatch(tag ->
tag.startsWith("Process - "));
+ assertTrue(!hasProcessPrefix);
Review Comment:
The assertion `assertTrue(!hasProcessPrefix)` is using double negation which
reduces readability. Use `assertFalse(hasProcessPrefix)` instead for clearer
intent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
