Skip to content

Conversation

@wuchunfu
Copy link
Member

@wuchunfu wuchunfu commented Sep 6, 2024

Purpose of this pull request

Change JobStatus to enum type to avoid hard coding

Check list

@Hisoka-X
Copy link
Member

Hisoka-X commented Sep 6, 2024

Please fix ci. cc @arshadmohammad

@wuchunfu
Copy link
Member Author

wuchunfu commented Sep 6, 2024

@Hisoka-X @arshadmohammad PTAL, thanks

@arshadmohammad
Copy link
Collaborator

arshadmohammad commented Sep 6, 2024

Many integration tests will fail after this change. @wuchunfu can you run the integration tests. Here is the command to run the integration tests

./mvnw -T 1C -B verify -DskipUT=true -DskipIT=false -DSEATUNNEL_HOME=/some/path/apache-seatunnel-2.3.7 -DST_WEB_BASEDIR_PATH=seatunnel-web-dist/target/apache-seatunnel-web-1.0.2-SNAPSHOT/apache-seatunnel-web-1.0.2-SNAPSHOT

@wuchunfu
Copy link
Member Author

wuchunfu commented Sep 7, 2024

Many integration tests will fail after this change. @wuchunfu can you run the integration tests. Here is the command to run the integration tests

./mvnw -T 1C -B verify -DskipUT=true -DskipIT=false -DSEATUNNEL_HOME=/some/path/apache-seatunnel-2.3.7 -DST_WEB_BASEDIR_PATH=seatunnel-web-dist/target/apache-seatunnel-web-1.0.2-SNAPSHOT/apache-seatunnel-web-1.0.2-SNAPSHOT

@arshadmohammad Thank you for your review. My testing is normal. Are you sure it was caused by my changes? If that's the case, is there a problem with CI? Can you fix it?

@wuchunfu
Copy link
Member Author

wuchunfu commented Sep 7, 2024

image

I didn't find any problems during my testing here

@Hisoka-X
Copy link
Member

Hisoka-X commented Sep 7, 2024

Many integration tests will fail after this change. @wuchunfu can you run the integration tests. Here is the command to run the integration tests
./mvnw -T 1C -B verify -DskipUT=true -DskipIT=false -DSEATUNNEL_HOME=/some/path/apache-seatunnel-2.3.7 -DST_WEB_BASEDIR_PATH=seatunnel-web-dist/target/apache-seatunnel-web-1.0.2-SNAPSHOT/apache-seatunnel-web-1.0.2-SNAPSHOT

@arshadmohammad Thank you for your review. My testing is normal. Are you sure it was caused by my changes? If that's the case, is there a problem with CI? Can you fix it?

We didn't bring test case into ci at now. We should add it.

@arshadmohammad
Copy link
Collaborator

@wuchunfu In your attached snapshot I can not see how many tests are run, how many failed or passed, can you please attach snapshot like below
image

Here I can see 42 tests are run, 0 failed, 0 error, 0 skipped.

@Hisoka-X
Copy link
Member

Hisoka-X commented Sep 7, 2024

@wuchunfu In your attached snapshot I can not see how many tests are run, how many failed or passed, can you please attach snapshot like below image

Here I can see 42 tests are run, 0 failed, 0 error, 0 skipped.

Hi @arshadmohammad , does our test case need rely mysql container? If not, let me add it into ci.

@arshadmohammad
Copy link
Collaborator

The integration test cases are dependent on both MySQL and the Seatunnel-Engine.

  1. Runtime mysql is being added as part of [Improvement][Seatunnel-web] Add mysql environment in e2e test cases. seatunnel#7403 to avoid dependency on external mysql.
    I will start working on this, so we can run IT test cases in CI environment.
  2. Integration tests also require seatunnel-engine which should be available in CI environment.

@Hisoka-X
Copy link
Member

Hisoka-X commented Sep 7, 2024

I will start working on this, so we can run IT test cases in CI environment.

Thanks @arshadmohammad ! You can refer https://github.com/apache/seatunnel/blob/dev/.github/workflows/build_main.yml and apache/seatunnel#5495. We implemented the logic to run CI in forked repositories so that devs can trigger CI themselves instead of maintainers manually triggering it every time.

Integration tests also require seatunnel-engine which should be available in CI environment.

Yes, we can use github action actions/checkout@v4.

@wuchunfu
Copy link
Member Author

wuchunfu commented Sep 7, 2024

@wuchunfu In your attached snapshot I can not see how many tests are run, how many failed or passed, can you please attach snapshot like below image

Here I can see 42 tests are run, 0 failed, 0 error, 0 skipped.

image

@arshadmohammad PTAL, thanks

Copy link
Collaborator

@arshadmohammad arshadmohammad left a comment

Choose a reason for hiding this comment

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

LGTM +1

@Hisoka-X Hisoka-X merged commit 94d8a39 into apache:main Sep 7, 2024
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.

3 participants