Enable notification integrationtest #376

gciavarrini · 2023-05-26T08:13:32Z

What this PR does / why we need it:
Enable notification service integration tests.
NotificationRecordTest has been refactored to match project code style/implementation.
In addition to that, it was necessary to fix the test to let it work with the integration tests in CI.

Future PRs will improve the NotificationRecordTest implementation.

Documentation PRs:

Fixes # FLPATH-348

Change type

Impacted services

Workflow Service
Notification Service

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.

...rc/test/java/com/redhat/parodos/integration/notification/common/NotificationTestBuilder.java

masayag · 2023-05-26T15:07:23Z

hack/manifests/testing/ingress.yaml

 spec:
  rules:
  - http:
      paths:
      - pathType: Prefix
-        path: /
+        path: /workflow-service(/|$)(.*)


Shouldn't this change require also an update or a similar configuration on the UI?

I do not think that it's the good way, then your swaggers configs are out of control, each service should have his own ingress.

masayag · 2023-05-26T15:07:35Z

hack/manifests/testing/ingress.yaml

        backend:
          service:
            name: workflow-service
            port:
              number: 8080
+      - pathType: Prefix
+        path: /notification-service(/|$)(.*)


same for this.

You're right!

WDYT?
https://github.com/gciavarrini/backstage-parodos/tree/add-testing-services-paths
See commit parodos-dev/backstage-parodos@main...gciavarrini:backstage-parodos:add-testing-services-paths

( I haven't opened the PR yet, I'd like to have an early feedback)

seems good to me, but requires verification and feedback from parodos-backstage maintainers.

...rc/test/java/com/redhat/parodos/integration/notification/common/NotificationTestBuilder.java

masayag · 2023-05-28T09:31:57Z

sdk-utils/src/main/java/com/redhat/parodos/sdkutils/WorkFlowServiceSdkUtils.java

@@ -36,9 +36,9 @@
 */

 @Slf4j
-public abstract class SdkUtils {
+public abstract class WorkFlowServiceSdkUtils {


can we go with: WorkFlowServiceUtils ?
sdk is already implied by the package name.

Updated. I also changed the NotificationServiceSdkUtils according to your suggestion.

hack/manifests/testing/parodos-patch.yaml

sdk-utils/src/main/java/com/redhat/parodos/sdkutils/NotificationServiceSdkUtils.java

nirarg · 2023-05-29T06:39:21Z

sdk-utils/src/main/java/com/redhat/parodos/sdkutils/NotificationServiceSdkUtils.java

+			throws ApiException, MissingRequiredPropertiesException, InterruptedException {
+		ApiClient apiClient = Configuration.getDefaultApiClient();
+		String serverIp = Optional.ofNullable(System.getenv("SERVER_IP")).orElse("localhost");
+		String serverPort = Optional.ofNullable(System.getenv("SERVER_PORT")).orElse("8080");


Default port 8081

sdk-utils/src/main/java/com/redhat/parodos/sdkutils/NotificationServiceSdkUtils.java

...-tests/src/test/java/com/redhat/parodos/integration/notification/NotificationRecordTest.java

nirarg · 2023-05-29T07:09:17Z

...rc/test/java/com/redhat/parodos/integration/notification/common/NotificationTestBuilder.java

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertTrue;
+
+public class NotificationTestBuilder {


There are duplicates between NotificationTestBuilder to WorkFlowTestBuilder
Consider to move those duplicates to common parent abstract class
(public class NotificationTestBuilder extends BaseTestBuilder)

The problem is that the duplicate code relies on objects that seems the same but that belong to different packages (for example com.redhat.parodos.sdkutils.ApiClient and com.redhat.parodos.sdk.invoker.ApiClient`). Such generated classes don't implement any common interface, so it's difficult to extract an abstract class.

Maybe in another PR we can try to write a wrapper for such sdk generated classes to reduce the code duplication. WDYT?

To reflect changes made in `parodos` PR parodos-dev/parodos#376 When deploying `parodos` locally or in a `Kind` cluster using `kubectl kustomize hack/manifests/testing | kubectl apply -f -` the workflow service and the notification service paths have been changed. Depends on PR parodos-dev/parodos#376 Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

To reflect changes in PR parodos-dev/parodos#376 Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

masayag · 2023-06-06T13:09:43Z

/lgtm

masayag · 2023-06-06T13:10:16Z

There are going to be conflicts with #399
First one to merge, wins :-)

To reflect changes in PR parodos-dev/parodos#376 Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

openshift-ci · 2023-06-06T15:30:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pkliczewski

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pkliczewski]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

To reflect changes in PR parodos-dev/parodos#376 Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

openshift-ci bot requested review from masayag and pkliczewski May 26, 2023 08:13

pkliczewski reviewed May 26, 2023

View reviewed changes

...rc/test/java/com/redhat/parodos/integration/notification/common/NotificationTestBuilder.java Outdated Show resolved Hide resolved

masayag reviewed May 26, 2023

View reviewed changes

masayag reviewed May 28, 2023

View reviewed changes

nirarg reviewed May 29, 2023

View reviewed changes

gciavarrini force-pushed the enable-notification-integrationtest branch from 39b4ede to c2cd035 Compare June 5, 2023 13:12

gciavarrini mentioned this pull request Jun 5, 2023

WIP: Update services paths parodos-dev/backstage-parodos#151

Closed

pkliczewski added the needs-rebase label Jun 6, 2023

Rename SdkUtils to WorkFlowServiceSdkUtils

b79dacb

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

gciavarrini force-pushed the enable-notification-integrationtest branch from c2cd035 to f3ab1e9 Compare June 6, 2023 10:39

openshift-merge-robot removed the needs-rebase label Jun 6, 2023

gciavarrini force-pushed the enable-notification-integrationtest branch from f3ab1e9 to 8e52d8f Compare June 6, 2023 11:53

gciavarrini added 13 commits June 6, 2023 14:09

Fix: port value check

e837ae6

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Re-enable notification integration tests

9eb0007

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Create NotificationServiceSdkUtils

5fa4132

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Cleanup notification before each test

c631da7

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Add notification service to ingress

199e467

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Add NotificationTestBuilder

f73739c

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Use NotificationTestBuilder in NotificationRecordTest

5f4aafc

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Add optional service path env var to workflow-service

fa60ead

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Update integration test command for CI

96c55d0

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Rename WorkFlowServiceSdkUtils to WorkFlowServiceUtils

cd82779

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Rename NotificationServiceSdkUtils to NotificationServiceUtils

f4c4eb2

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Update assertion style

e1671cd

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Return void in waitNotificationStart

4402522

Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

gciavarrini force-pushed the enable-notification-integrationtest branch from 8e52d8f to 4402522 Compare June 6, 2023 12:12

gciavarrini requested a review from masayag June 6, 2023 12:19

gciavarrini requested a review from eloycoto June 6, 2023 12:19

gciavarrini added a commit to gciavarrini/parodos-dev.github.io that referenced this pull request Jun 6, 2023

Update how to run integration tests command

c91e568

To reflect changes in PR parodos-dev/parodos#376 Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

masayag requested review from nirarg and pkliczewski June 6, 2023 13:05

openshift-ci bot assigned masayag Jun 6, 2023

openshift-ci bot added the lgtm label Jun 6, 2023

gciavarrini added a commit to gciavarrini/parodos-dev.github.io that referenced this pull request Jun 6, 2023

Update how to run integration tests command

7fa02e3

To reflect changes in PR parodos-dev/parodos#376 Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

gciavarrini mentioned this pull request Jun 6, 2023

Update how to run integration tests command parodos-dev/parodos-dev.github.io#17

Merged

gciavarrini added a commit to gciavarrini/parodos-dev.github.io that referenced this pull request Jun 6, 2023

Update how to run integration tests command

800ea84

To reflect changes in PR parodos-dev/parodos#376 Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

pkliczewski approved these changes Jun 6, 2023

View reviewed changes

openshift-ci bot added the approved label Jun 6, 2023

openshift-merge-robot merged commit f11110c into parodos-dev:main Jun 6, 2023

gciavarrini added a commit to gciavarrini/parodos-dev.github.io that referenced this pull request Jun 7, 2023

Update how to run integration tests command

3e27b68

To reflect changes in PR parodos-dev/parodos#376 Signed-off-by: Gloria Ciavarrini <gciavarrini@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable notification integrationtest #376

Enable notification integrationtest #376

gciavarrini commented May 26, 2023 •

edited

Loading

masayag May 26, 2023

eloycoto Jun 5, 2023

masayag May 26, 2023

gciavarrini Jun 5, 2023

masayag Jun 5, 2023

masayag May 28, 2023

gciavarrini Jun 5, 2023

nirarg May 29, 2023

nirarg May 29, 2023 •

edited

Loading

gciavarrini Jun 5, 2023

masayag commented Jun 6, 2023

masayag commented Jun 6, 2023

openshift-ci bot commented Jun 6, 2023

Enable notification integrationtest #376

Enable notification integrationtest #376

Conversation

gciavarrini commented May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirarg May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayag commented Jun 6, 2023

masayag commented Jun 6, 2023

openshift-ci bot commented Jun 6, 2023

gciavarrini commented May 26, 2023 •

edited

Loading

nirarg May 29, 2023 •

edited

Loading