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.

Sign up for GitHub

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

Jump to bottom

ZOOKEEPER-4780: Delegate temp directory creation to Junit in tests. #2100

Merged
merged 6 commits into from
Feb 9, 2024
Merged

ZOOKEEPER-4780: Delegate temp directory creation to Junit in tests. #2100

merged 6 commits into from
Feb 9, 2024

Conversation

muthu90tech
Copy link
Contributor

@muthu90tech muthu90tech commented Dec 22, 2023 •
edited
Loading

Jira: https://issues.apache.org/jira/browse/ZOOKEEPER-4780

Use Junit @tempdir annotation to manage temp directory for tests.
Junit creates the tempDir automatically and cleans them after test. Which does not clutter the src/test directories.

@muthu90tech
ZOOKEEPER-4780: Delegate temp directory creation to Junit in tests.
787feed 
Use Junit @tempdir annotation to manage temp directory for tests.
@muthu90tech
Use TempDir annotation for few more tests.
b1cb458
@muthu90tech
Remove unusedimports
641a23d 
There is a test which fails on the CI, but when I reran it local it passes. test=TruncateTest
@muthu90tech
Revert TruncateTest
547918d
ctubbsii
ctubbsii approved these changes Dec 27, 2023
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

LGTM

ztzg
ztzg reviewed Jan 4, 2024
View reviewed changes
Copy link
Contributor

@ztzg ztzg left a comment

Choose a reason for hiding this comment

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

LGTM, in general. A nice cleanup, indeed!

@TempDir is marked STABLE starting in JUnit 5.10, so depending on it sounds reasonable.

Some specific call sites might be worth another round of review, or perhaps even an update, though; see my individual comments.

zookeeper-server/src/test/java/org/apache/zookeeper/server/PrepRequestProcessorTest.java Outdated Show resolved Hide resolved
zookeeper-server/src/test/java/org/apache/zookeeper/server/admin/JettyAdminServerTest.java Outdated Show resolved Hide resolved
zookeeper-server/src/test/java/org/apache/zookeeper/server/controller/ControllerTestBase.java Show resolved Hide resolved
zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/SnapStreamTest.java Show resolved Hide resolved
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/CommitProcessorTest.java Outdated Show resolved Hide resolved
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/Zab1_0Test.java Outdated Show resolved Hide resolved
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/auth/QuorumAuthTestBase.java Outdated
@@ -44,11 +44,11 @@ public class QuorumAuthTestBase extends ZKTestCase {

protected static final Logger LOG = LoggerFactory.getLogger(QuorumAuthTestBase.class);
protected List<MainThread> mt = new ArrayList<>();
@TempDir
protected static File jaasConfigDir;
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Unless I am missing something, this value is used by a static method, which is itself called by static { } blocks in derived classes. How do we know the JUnit "engine" is going to be able to initialize the field before such blocks are executed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a great, question. I dont know.. This will need some investigation. I will look into this. Meantime I am going to revert this and avoid using the annotation only for this. But its a good deep dive I think..

Copy link
Member

Choose a reason for hiding this comment

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

Should avoid static initializer blocks in general... they make for hard-to-test code, not to mention the potential bootstrapping issue mentioned here. Should use JUnit @BeforeAll annotation to initialize static fields in test case classes, if possible, to make sure JUnit does the right thing automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created this https://issues.apache.org/jira/browse/ZOOKEEPER-4789 to work on cleaning the QuorumAuth tests.

@muthu90tech
Copy link
Contributor Author

LGTM, in general. A nice cleanup, indeed!

@TempDir is marked STABLE starting in JUnit 5.10, so depending on it sounds reasonable.

Some specific call sites might be worth another round of review, or perhaps even an update, though; see my individual comments.

Thank you, will take a look and get back on this.

@muthu90tech
Address comments
b974022 
Make sure the temp directory scope is not affected.
@muthu90tech
fix for ZKClientConfigTest
ae177f2
@muthu90tech muthu90tech requested a review from ztzg January 21, 2024 16:37
eolivelli
eolivelli requested changes Feb 3, 2024
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

I hae left one comment about the Zab01 test, apart from that LGTM

zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/Zab1_0Test.java Show resolved Hide resolved
@muthu90tech muthu90tech requested a review from eolivelli February 3, 2024 18:35
@eolivelli eolivelli requested a review from ctubbsii February 5, 2024 07:42
eolivelli
eolivelli approved these changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

thanks for your contribution

@anmolnar anmolnar merged commit e10bf86 into apache:master Feb 9, 2024
13 checks passed
@anmolnar anmolnar mentioned this pull request Feb 9, 2024
Merged
@anmolnar
Copy link
Contributor

anmolnar commented Feb 9, 2024

Thanks @muthu90tech , what's your jira id?
I'd like to add you as contributor.

@muthu90tech
Copy link
Contributor Author

Thanks @muthu90tech , what's your jira id? I'd like to add you as contributor.

Thanks @anmolnar , JiraID: muthu90tech

@anmolnar
Copy link
Contributor

anmolnar commented Feb 9, 2024

@muthu90tech Great. Now you're a ZooKeeper contributor. ;)

@muthu90tech
Copy link
Contributor Author

@muthu90tech Great. Now you're a ZooKeeper contributor. ;)

Awesome, looking forward to this!

AlphaCanisMajoris pushed a commit to AlphaCanisMajoris/zookeeper that referenced this pull request Mar 28, 2024
@muthu90tech @AlphaCanisMajoris
ZOOKEEPER-4780: Delegate temp directory creation to Junit in tests.
25340d3 
ZOOKEEPER-4780: Delegate temp directory creation to Junit in tests.
Use Junit @tempdir annotation to manage temp directory for tests.
Use TempDir annotation for few more tests.
Remove unusedimports
There is a test which fails on the CI, but when I reran it local it passes. test=TruncateTest
Revert TruncateTest
Address comments
Make sure the temp directory scope is not affected.
fix for ZKClientConfigTest
Reviewers: ctubbsii, eolivelli
Author: muthu90tech
Closes apache#2100 from muthu90tech/ZOOKEEPER-4780
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tisonkun tisonkun tisonkun left review comments

@eolivelli eolivelli eolivelli approved these changes

@ztzg ztzg Awaiting requested review from ztzg

@ctubbsii ctubbsii Awaiting requested review from ctubbsii

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@muthu90tech @anmolnar @ztzg @ctubbsii @eolivelli @tisonkun