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

[Minor] refactor test code #432

Merged
merged 2 commits into from
Dec 21, 2022
Merged

Conversation

advancedxy
Copy link
Contributor

@advancedxy advancedxy commented Dec 16, 2022

What changes were proposed in this pull request?

Refactor some test code

Why are the changes needed?

For better code quality. And fixes some flaky test such as #388

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing UTs. There should be no logic change.

ShuffleServerConf conf = new ShuffleServerConf();
conf.setBoolean(ShuffleServerConf.HEALTH_CHECK_ENABLE, true);
conf.setString(ShuffleServerConf.RSS_STORAGE_TYPE, StorageType.LOCALFILE.name());
conf.set(ShuffleServerConf.RSS_STORAGE_BASE_PATH, Arrays.asList("st1", "st2", "st3"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This creates empty dirs: st1,st2,st3 under repo dir when running in IDEs.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Could u help do some cleanup after test finished?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After this change. The st1, st2, st3 should be deleted automatically since they are guarded by the @TempDir annotation.

Is there anything I'm missing here?

Copy link
Member

Choose a reason for hiding this comment

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

You are right.

@advancedxy advancedxy marked this pull request as draft December 16, 2022 07:53
@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2022

Codecov Report

Merging #432 (7e008dc) into master (7b633c0) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #432      +/-   ##
============================================
+ Coverage     58.53%   58.61%   +0.07%     
- Complexity     1615     1619       +4     
============================================
  Files           195      195              
  Lines         11042    11044       +2     
  Branches        973      973              
============================================
+ Hits           6463     6473      +10     
+ Misses         4202     4195       -7     
+ Partials        377      376       -1     
Impacted Files Coverage Δ
...g/apache/uniffle/common/compression/NoOpCodec.java 100.00% <100.00%> (+100.00%) ⬆️
...a/org/apache/uniffle/common/compression/Codec.java 100.00% <0.00%> (+16.66%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zuston
zuston previously approved these changes Dec 16, 2022
Copy link
Member

@zuston zuston left a comment

Choose a reason for hiding this comment

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

LGTM.

I will approve first. If you have some changes, please amend them

@advancedxy advancedxy marked this pull request as ready for review December 21, 2022 06:17
@advancedxy advancedxy changed the title [WIP][Minor] refactor test code [Minor] refactor test code Dec 21, 2022
@advancedxy
Copy link
Contributor Author

ping @zuston and @jerqi. There might be other small refactors to be done, but let's do that in other RPs.

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@zuston zuston left a comment

Choose a reason for hiding this comment

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

LGTM

@advancedxy advancedxy merged commit 916f0ca into apache:master Dec 21, 2022
@advancedxy
Copy link
Contributor Author

Thanks. Merged.

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.

None yet

4 participants