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

[#3024] improvement(IT): close containers and upload container log after all tests are finished #3197

Merged
merged 6 commits into from
Apr 29, 2024

Conversation

xiaozcy
Copy link
Contributor

@xiaozcy xiaozcy commented Apr 28, 2024

What changes were proposed in this pull request?

close containers and upload container log after all tests are finished

Why are the changes needed?

Fix: #3024

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

existing ITs

@xiaozcy xiaozcy removed their assignment Apr 28, 2024
@xiaozcy xiaozcy marked this pull request as draft April 28, 2024 13:13
@xiaozcy xiaozcy closed this Apr 28, 2024
@xiaozcy xiaozcy reopened this Apr 28, 2024
@xiaozcy xiaozcy marked this pull request as ready for review April 29, 2024 01:37
@xiaozcy
Copy link
Contributor Author

xiaozcy commented Apr 29, 2024

@yuqi1129 This PR is ready for review. BTW, can you add upload log label to this PR so that we could verify if the logs are uploaded as expected?

@yuqi1129 yuqi1129 added the upload log Always upload container log label Apr 29, 2024
@yuqi1129 yuqi1129 closed this Apr 29, 2024
@yuqi1129 yuqi1129 reopened this Apr 29, 2024
qqqttt123 pushed a commit that referenced this pull request Apr 29, 2024
…ntegration-test-common` (#3201)

### What changes were proposed in this pull request?

Trigger integration test when there is a change in module
`integration-test-common`.

### Why are the changes needed?

We should start the integration test if there are changes in module
`integration-test-common`. however, CI
https://github.com/datastrato/gravitino/actions/runs/8872110159/job/24356012525?pr=3197
of PR #3197 can't start the
CI pipeline.

Fixed: #3207 

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

CI passed.
github-actions bot pushed a commit that referenced this pull request Apr 29, 2024
…ntegration-test-common` (#3201)

### What changes were proposed in this pull request?

Trigger integration test when there is a change in module
`integration-test-common`.

### Why are the changes needed?

We should start the integration test if there are changes in module
`integration-test-common`. however, CI
https://github.com/datastrato/gravitino/actions/runs/8872110159/job/24356012525?pr=3197
of PR #3197 can't start the
CI pipeline.

Fixed: #3207 

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

CI passed.
qqqttt123 pushed a commit that referenced this pull request Apr 29, 2024
…ntegration-test-common` (#3208)

### What changes were proposed in this pull request?

Trigger integration test when there is a change in module
`integration-test-common`.

### Why are the changes needed?

We should start the integration test if there are changes in module
`integration-test-common`. however, CI
https://github.com/datastrato/gravitino/actions/runs/8872110159/job/24356012525?pr=3197
of PR #3197 can't start the
CI pipeline.

Fixed: #3207 

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

CI passed.

Co-authored-by: Qi Yu <[email protected]>
@yuqi1129
Copy link
Contributor

@xiaozcy
It seems fine if all pipelines succeed, please update the code to test if everything is okay with a failure.

@xiaozcy
Copy link
Contributor Author

xiaozcy commented Apr 29, 2024

@xiaozcy It seems fine if all pipelines succeed, please update the code to test if everything is okay with a failure.

@yuqi1129 I use #3213 to test if logs are uploaded as expected when some tests failed, the results indicate that everything is fine in this situation.
#3213 should be closed after you check the results.

@yuqi1129
Copy link
Contributor

@xiaozcy It seems fine if all pipelines succeed, please update the code to test if everything is okay with a failure.

@yuqi1129 I use #3213 to test if logs are uploaded as expected when some tests failed, the results indicate that everything is fine in this situation. #3213 should be closed after you check the results.

I see, thanks for your work.

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class CloseContainerExtension implements BeforeAllCallback {
Copy link
Contributor

Choose a reason for hiding this comment

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

@xiaozcy
Adding a comment about this class is preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@yuqi1129
Copy link
Contributor

LGTM except for one small problem.

Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

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

LGTM

@yuqi1129 yuqi1129 added need backport Issues that need to backport to another branch branch-0.5 labels Apr 29, 2024
@jerryshao jerryshao merged commit 91c63f9 into apache:main Apr 29, 2024
20 checks passed
github-actions bot pushed a commit that referenced this pull request Apr 29, 2024
…ter all tests are finished (#3197)

### What changes were proposed in this pull request?

close containers and upload container log after all tests are finished

### Why are the changes needed?

Fix: #3024 

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

existing ITs

---------

Co-authored-by: zhanghan18 <[email protected]>
Co-authored-by: Qi Yu <[email protected]>
diqiu50 pushed a commit to diqiu50/gravitino that referenced this pull request Jun 13, 2024
…ule `integration-test-common` (apache#3201)

### What changes were proposed in this pull request?

Trigger integration test when there is a change in module
`integration-test-common`.

### Why are the changes needed?

We should start the integration test if there are changes in module
`integration-test-common`. however, CI
https://github.com/datastrato/gravitino/actions/runs/8872110159/job/24356012525?pr=3197
of PR apache#3197 can't start the
CI pipeline.

Fixed: apache#3207 

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

CI passed.
diqiu50 pushed a commit to diqiu50/gravitino that referenced this pull request Jun 13, 2024
…log after all tests are finished (apache#3197)

### What changes were proposed in this pull request?

close containers and upload container log after all tests are finished

### Why are the changes needed?

Fix: apache#3024 

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

existing ITs

---------

Co-authored-by: zhanghan18 <[email protected]>
Co-authored-by: Qi Yu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need backport Issues that need to backport to another branch upload log Always upload container log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Subtask] Optimize upload logs of IT container
4 participants