Improvements to the junit extension #891

andreaTP · 2022-01-31T18:25:17Z

This PR integrates a few desired features into the current junit extension, without breaking the backward compatibility.

...mysql-schema/src/test/java/io/javaoperatorsdk/operator/sample/MySQLSchemaOperatorNewE2E.java

andreaTP · 2022-02-01T11:38:33Z

Those changes are largely inspired by @jonathanvila on this PR: https://github.com/keycloak/keycloak/pull/9625/files

andreaTP · 2022-02-01T11:44:43Z

Added a pipeline of the "mysql" sample to showcase the usage and benefits.
I do believe that those changes are reducing the complexity of the user's code and setup while they are not dramatically increasing the complexity in this codebase.

operator-framework-core/pom.xml

andreaTP · 2022-02-01T11:54:15Z

Happy to have a review of those proposed changes @csviri @metacosm

operator-framework-core/pom.xml

andreaTP · 2022-02-02T15:12:01Z

NDR: rebased on latest main and Sonar check is correctly skipped @csviri

...ator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/OperatorExtension.java

lburgazzoli · 2022-02-03T09:26:32Z

...ator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/OperatorExtension.java

+    this.operatorDeployment = operatorDeployment;
+    this.deploymentTimeout = deploymentTimeout;
+    this.infrastructure = infrastructure;
+    this.infrastructureTimeout = infrastructureTimeout;
    this.operator = new Operator(this.kubernetesClient, this.configurationService);


maybe this should be passed in the constructor and created by the builder I don't think there is a case where you need both a local operator and a deployed operator

...ator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/OperatorExtension.java

csviri · 2022-02-03T09:36:09Z

Conceptually we are mixing here the Integration and E2E tests. In general not opposed to this. But not sure if these should not be two different extensions, in Integration tests we don't usually use other resources (e.g. infrastructure) we don't deploy of course the operator etc. Maybe having a hierarchy between these two extension? like a common base class, if we need to share code.
So in short these are little different domain, with different needs, this way we provide a mixed api for the two domains. Separating it to two extensions would make it more explicit.

But don't have strong opinion just an idea.

lburgazzoli · 2022-02-03T09:53:38Z

Conceptually we are mixing here the Integration and E2E tests. In general not opposed to this. But not sure if these should not be two different extensions, in Integration tests we don't usually use other resources (e.g. infrastructure) we don't deploy of course the operator etc. Maybe having a hierarchy between these two extension? like a common base class, if we need to share code. So in short these are little different domain, with different needs, this way we provide a mixed api for the two domains. Separating it to two extensions would make it more explicit.

But don't have strong opinion just an idea.

I don't have a strong opinion too but I think this is a valid point.

Maybe (but it can be done in a follow up PR) we should end up with two different extensions: one that deals with an embedded operator and one with a deployed one.

andreaTP · 2022-02-03T09:55:18Z

@csviri yes, indeed, we are intentionally mixing integration and E2E tests, the objective is to enable this workflow:

locally develop the operator and the tests running from your machine (integration) - fast iteration - missing checks on k8s (such as RBAC etc.)
in CI using the very same test/code and a simple switch from command line (E2E) - possibly slower - very close if not identical to the final product

I'm not opposed to split the implementation using two subclasses but I would really like to make it easy to "switch" integration/E2E keeping the same(or almost) test sources.

andreaTP · 2022-02-03T15:11:24Z

Separated into a common abstract class to refactor common functionalities and two sub-classes OperatorExtension and E2EOperatorExtension.
This actually makes everything more clean, great suggestions thanks!

I think this is ready for another round of review 🙏 @lburgazzoli @csviri

csviri

LGTM, I think we can remove the old tests and the related workflow and merge it

andreaTP · 2022-02-04T08:36:04Z

@csviri thanks! 👍
I'll clean up the sample operators tests and remove the old "kind" approach in favour of the E2EOperatorExtension setup, anything else that should be cleaned up?

andreaTP · 2022-02-04T10:32:47Z

cleaned up
ported the E2E of the Tomcat operator to this new structure
unified the E2E workflows that are now using a GH action matrix
got a green CI 🍏

Should be good for a final check @csviri 🙂

.github/workflows/e2e-test.yml

metacosm · 2022-02-04T12:01:49Z

.github/workflows/e2e-test.yml

+        uses: actions/setup-java@v2
+        with:
+          java-version: 11
+          distribution: adopt-hotspot


Is using a different distribution from the tests on purpose here? Might actually be a good idea since it improves somewhat VM coverage?

adopt rebranded the distribution to temurin, so adopt v17 doesn't exists (it's temurin now).

Switched to temurin, I "think" that, in this repository, we are not leveraging sensible parts of the JVM and we should not need to test against different distributions ... but open to suggestions!

metacosm

LGTM, will open a subsequent PR to improve things a little

andreaTP mentioned this pull request Jan 31, 2022

showcase how to test operator #860

Closed

andreaTP commented Jan 31, 2022

View reviewed changes

...mysql-schema/src/test/java/io/javaoperatorsdk/operator/sample/MySQLSchemaOperatorNewE2E.java Outdated Show resolved Hide resolved

andreaTP commented Jan 31, 2022

View reviewed changes

...mysql-schema/src/test/java/io/javaoperatorsdk/operator/sample/MySQLSchemaOperatorNewE2E.java Outdated Show resolved Hide resolved

andreaTP force-pushed the improve-testing branch 3 times, most recently from e4dca5e to 8531e75 Compare February 1, 2022 11:37

andreaTP force-pushed the improve-testing branch from 8531e75 to e1940fd Compare February 1, 2022 11:44

andreaTP marked this pull request as ready for review February 1, 2022 11:52

andreaTP commented Feb 1, 2022

View reviewed changes

operator-framework-core/pom.xml Outdated Show resolved Hide resolved

andreaTP changed the title ~~[WIP] Improve the junit extension~~ Improvements to the junit extension Feb 1, 2022

andreaTP force-pushed the improve-testing branch from e1940fd to c731fd2 Compare February 1, 2022 15:51

metacosm reviewed Feb 1, 2022

View reviewed changes

operator-framework-core/pom.xml Outdated Show resolved Hide resolved

andreaTP force-pushed the improve-testing branch 3 times, most recently from d3d948a to a2025af Compare February 2, 2022 15:10

csviri requested a review from lburgazzoli February 3, 2022 08:49

lburgazzoli reviewed Feb 3, 2022

View reviewed changes

...ator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/OperatorExtension.java Outdated Show resolved Hide resolved

lburgazzoli reviewed Feb 3, 2022

View reviewed changes

csviri reviewed Feb 3, 2022

View reviewed changes

...ator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/OperatorExtension.java Outdated Show resolved Hide resolved

csviri reviewed Feb 3, 2022

View reviewed changes

...ator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/OperatorExtension.java Outdated Show resolved Hide resolved

csviri approved these changes Feb 4, 2022

View reviewed changes

lburgazzoli approved these changes Feb 4, 2022

View reviewed changes

andreaTP force-pushed the improve-testing branch from 100107e to eead2d4 Compare February 4, 2022 10:17

andreaTP force-pushed the improve-testing branch from eead2d4 to 4e7492c Compare February 4, 2022 11:26

metacosm reviewed Feb 4, 2022

View reviewed changes

.github/workflows/e2e-test.yml Outdated Show resolved Hide resolved

metacosm reviewed Feb 4, 2022

View reviewed changes

Enable easy E2E and improvements to the integration test suite

2b66241

andreaTP force-pushed the improve-testing branch from 4e7492c to 2b66241 Compare February 4, 2022 12:16

metacosm approved these changes Feb 4, 2022

View reviewed changes

metacosm merged commit ff1a44d into operator-framework:main Feb 4, 2022

Improvements to the junit extension #891

Improvements to the junit extension #891

Uh oh!

Conversation

andreaTP commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreaTP commented Feb 1, 2022

Uh oh!

andreaTP commented Feb 1, 2022

Uh oh!

Uh oh!

andreaTP commented Feb 1, 2022

Uh oh!

Uh oh!

andreaTP commented Feb 2, 2022

Uh oh!

Uh oh!

lburgazzoli Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

csviri commented Feb 3, 2022

Uh oh!

lburgazzoli commented Feb 3, 2022

Uh oh!

andreaTP commented Feb 3, 2022

Uh oh!

andreaTP commented Feb 3, 2022

Uh oh!

csviri left a comment

Choose a reason for hiding this comment

Uh oh!

andreaTP commented Feb 4, 2022

Uh oh!

andreaTP commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

metacosm Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

andreaTP Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

metacosm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andreaTP commented Jan 31, 2022 •

edited

Loading

andreaTP commented Feb 4, 2022 •

edited

Loading