Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SNOW-1269024 Installing multiple Java versions for test purposes #824

Conversation

sfc-gh-krosinski
Copy link
Collaborator

@sfc-gh-krosinski sfc-gh-krosinski commented Apr 23, 2024

Overview

SNOW-1269024 - adding verification on different versions of JRE
Building and unit testing process remains the same (Java8) but e2e testing is run on three different versions of JAVA: 8, 11 and 17. This means that there are 3 times more testing jobs, but they all run in parallel, hence total run time should remain ~the same.

Pre-review checklist

  • This change should be part of a Behavior Change Release. See go/behavior-change.
  • This change has passed Merge gate tests
  • Snowpipe Changes
  • Snowpipe Streaming Changes
  • This change is TEST-ONLY
  • This change is README/Javadocs only
  • This change is protected by a config parameter <PARAMETER_NAME> eg snowflake.ingestion.method.
    • Yes - Added end to end and Unit Tests.
    • No - This is test only change
  • Is his change protected by parameter <PARAMETER_NAME> on the server side?
    • The parameter/feature is not yet active in production (partial rollout or PrPr, see Changes for Unreleased Features and Fixes).
    • If there is an issue, it can be safely mitigated by turning the parameter off. This is also verified by a test (See go/ppp).

normal

Longer test runs

Copy link
Contributor

@sfc-gh-tzhang sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please see if @sfc-gh-japatel could take a look

@@ -18,9 +18,12 @@ jobs:
- name: Checkout Code
uses: actions/checkout@v2
- name: "Install Java 8"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update the name as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to go with different approach - here only Java 8 is installed, hence the name remains the same.

Added JAVA distribution as it no longer dafaults to 'zulu' and is required parameter.
Added JAVA distribution as it no longer dafaults to 'zulu' and is required parameter.
@sfc-gh-krosinski sfc-gh-krosinski marked this pull request as draft April 24, 2024 09:24
Changed approach to only install second JAVA - the one required for tests, matrix for test-JAVA version
Add verification of java used by print to log
Aligned step naming to contain matrix value used
Copy link
Collaborator

@sfc-gh-japatel sfc-gh-japatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any plans for confluent end to end test?

@@ -14,13 +14,15 @@ jobs:
matrix:
apache_kafka_version: [ '2.5.1', '2.8.1', '3.2.1' ]
snowflake_cloud: [ 'AWS', 'AZURE', 'GCS' ]
java_test_version: [ '8', '11', '17' ]
steps:
- name: Checkout Code
uses: actions/checkout@v2
- name: "Install Java 8"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this step? as in we are installing java 8 here but using different versions below.

Copy link
Collaborator Author

@sfc-gh-krosinski sfc-gh-krosinski Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole point of my change is to use two different versions of java - java 8 for building and unit testing but then 8, 11 and 17 as jre to run the app and verify it against given runtime environment - I added PRs description now as I switched it to "ready for review" so I hope it's more clear.

@sfc-gh-krosinski sfc-gh-krosinski marked this pull request as ready for review April 25, 2024 06:46
Copy link
Contributor

@sfc-gh-wtrefon sfc-gh-wtrefon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@sfc-gh-dseweryn sfc-gh-dseweryn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 💪

@sfc-gh-krosinski sfc-gh-krosinski merged commit 03151fc into master Apr 25, 2024
51 checks passed
@sfc-gh-krosinski sfc-gh-krosinski deleted the krosinski-SNOW-1269024-add-testing-matrix-to-end-2-end-test-apache-yml branch April 25, 2024 08:50
ConfluentSemaphore pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Aug 6, 2024
…wflakedb#824)

Building and unit testing process remains the same (Java8) but e2e testing is run on three different versions of JAVA: 8, 11 and 17. This means that there are 3 times more testing jobs, but they all run in parallel, hence total run time should remain ~the same.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants