[#2992] improvement(PyClient): Fix unstable Python client integration test #2994
Conversation
This file contains 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
jerryshao
reviewed
Apr 17, 2024
| fileset_propertie_value2: str = "fileset_propertie_value2" | ||
| fileset_properties: Dict[str, str] = {fileset_propertie_key1: fileset_propertie_value1, | ||
| fileset_propertie_key2: fileset_propertie_value2} | ||
| fileset_new_name = fileset_name + "_new" |
There was a problem hiding this comment.
It is better that you can add a random prefix to all the names so that you will not fail for any case, like what we did in Java IT?
|
@xunliu |
jerryshao
reviewed
Apr 18, 2024
Comment on lines
69
to
71
| echo "Use Python version ${pythonVersion} to test the Python client." | ||
| # Start Gravitino server and set the environment variable | ||
| ./distribution/package/bin/gravitino.sh start |
There was a problem hiding this comment.
With this manual startUp step, how do we test in local environment using gradle?
There was a problem hiding this comment.
- Test Principle: Every Python ITs class base on the
IntegrationTestEnv,IntegrationTestEnvwill automatic start and stop Gravitino server to support ITs, But when you run multiple ITs class at same time, The first test class that finishes running will shut down the Gravitino server, which will cause other test classes to fail if they can't connect to the Gravitino server. - Run test in the IDE: Through
IntegrationTestEnvclass automatic start and stop Gravitino server to support ITs. - Run test in the Github Action: Manual start and stop Gravitino server, and set
EXTERNAL_START_GRAVITINOenvironment variable - Run test in the Gradle command
:client:client-python:test: Gradle automatic start and stop Gravitino server, and setEXTERNAL_START_GRAVITINOenvironment variable.
I will create a document to description this in the next PR.
jerryshao
reviewed
Apr 18, 2024
jerryshao
reviewed
Apr 18, 2024
| - name: Python Client Integration Test | ||
| id: integrationTest | ||
| env: | ||
| EXTERNAL_START_GRAVITINO: true |
There was a problem hiding this comment.
Better change to “START_EXTERNAL_GRAVITINO”
| val skipPyClientITs = project.hasProperty("skipPyClientITs") | ||
| if (!skipPyClientITs) { | ||
| doFirst { | ||
| gravitinoServer("start") |
There was a problem hiding this comment.
Do we still need to start Gravitino manually if it is started manually in CI?
There was a problem hiding this comment.
Yes, You are right, I removed this code.
| doLast { | ||
| gravitinoServer("stop") | ||
| doLast { | ||
| gravitinoServer("stop") |
|
@jerryshao Please help me review this PR, Thanks. |
jerryshao
changed the title
diqiu50
pushed a commit
to diqiu50/gravitino
that referenced
this pull request
Jun 13, 2024
…ration test (apache#2994) ### What changes were proposed in this pull request? 1. Add `skipPyClientITs` param in Gradle build script. Let's Python client integration test not running in the backend ITs 2. Combined Gradle build command of Python client test and integration test 3. improvement FilesetCatalog integration test codes. 4. Test mode + Test Principle: Every Python ITs class base on the `IntegrationTestEnv`, `IntegrationTestEnv` will automatic start and stop Gravitino server to support ITs, But when you run multiple ITs class at same time, The first test class that finishes running will shut down the Gravitino server, which will cause other test classes to fail if they can't connect to the Gravitino server. + Run test in the IDE: Through `IntegrationTestEnv` class automatic start and stop Gravitino server to support ITs. + Run test in the Github Action: Manual start and stop Gravitino server, and set `EXTERNAL_START_GRAVITINO` environment variable + Run test in the Gradle command `:client:client-python:test`: Gradle automatic start and stop Gravitino server, and set `EXTERNAL_START_GRAVITINO` environment variable. ### Why are the changes needed? Fix: apache#2292 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? Through CI check.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What changes were proposed in this pull request?
Add
skipPyClientITsparam in Gradle build script. Let's Python client integration test not running in the backend ITsCombined Gradle build command of Python client test and integration test
improvement FilesetCatalog integration test codes.
Test mode
IntegrationTestEnv,IntegrationTestEnvwill automatic start and stop Gravitino server to support ITs, But when you run multiple ITs class at same time, The first test class that finishes running will shut down the Gravitino server, which will cause other test classes to fail if they can't connect to the Gravitino server.IntegrationTestEnvclass automatic start and stop Gravitino server to support ITs.EXTERNAL_START_GRAVITINOenvironment variable:client:client-python:test: Gradle automatic start and stop Gravitino server, and setEXTERNAL_START_GRAVITINOenvironment variable.Why are the changes needed?
Fix: #2992
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
Through CI check.