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

[Improve][SeaTunnel-Web] Change JobStatus to enum type to avoid hard coding #205

Merged
merged 4 commits into from
Sep 7, 2024

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
15 checks passed
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