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

Aliyun: Switch iceberg-aliyun's tests to Junit5 #9122

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

lisirrx
Copy link
Contributor

@lisirrx lisirrx commented Nov 21, 2023

Switch iceberg-aliyun's tests to Junit5
Fix: #9081
@nastra Hi, I have changed all of the junit4 code to junit5 with AssertJ, but I have a question here: To fit the Extension class of Junit5, I changed most of the term 'Rule' to 'Extension', but there is a system env variable in org/apache/iceberg/aliyun/TestUtility.java called ALIYUN_TEST_OSS_TEST_RULE_CLASS.
I don't know where the CI system runs, and how the variable is set. So I'm not sure if I should change this.

Change Junit4 Assume to Junit5 Assumptions
Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

@lisirrx thanks for working on this. I left a few inital comments and I'll take a closer look tomorrow at the issue around TestUtility

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

just a few small things to fix, but overall this LGTM.

I also checked how the env variables are being used and they are not set by our CI. They were introduced by #3687, so pinging @openinx to also have a look at the proposed changes here.

@lisirrx
Copy link
Contributor Author

lisirrx commented Nov 22, 2023

just a few small things to fix, but overall this LGTM.

I also checked how the env variables are being used and they are not set by our CI. They were introduced by #3687, so pinging @openinx to also have a look at the proposed changes here.

Thank you for your patient review. I'm sorry for introducing unnecessary blank lines. I will pay attention to this issue when writing code in the future.
If I understand correctly, will the integration test of this package only run locally on the developer's computer? I ran these tests successfully with my own Aliyun AK/SK. Maybe let's wait for @openinx ‘s opinion.

Btw, should I squash the commits together or you will do that when merging the pr?

@nastra
Copy link
Contributor

nastra commented Nov 22, 2023

Btw, should I squash the commits together or you will do that when merging the pr?

commits will be squashed when the PR is getting merged, so it's up to you if you want to squash them or not.

@lisirrx
Copy link
Contributor Author

lisirrx commented Nov 24, 2023

Btw, should I squash the commits together or you will do that when merging the pr?

commits will be squashed when the PR is getting merged, so it's up to you if you want to squash them or not.

hi, @nastra, any addtional work should I do for this PR to move forward?

@nastra
Copy link
Contributor

nastra commented Nov 24, 2023

Btw, should I squash the commits together or you will do that when merging the pr?

commits will be squashed when the PR is getting merged, so it's up to you if you want to squash them or not.

hi, @nastra, any addtional work should I do for this PR to move forward?

Let's give @openinx a few days to review this PR

@nastra nastra merged commit b4c050b into apache:main Dec 5, 2023
45 checks passed
lisirrx added a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iceberg-aliyun: Switch tests to JUnit5 + AssertJ-style assertions
2 participants