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

CURATOR-633: Fix backward compatibility in tests #409

Closed
wants to merge 3 commits into from
Closed

CURATOR-633: Fix backward compatibility in tests #409

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1bcc5c
CURATOR-633: Run o.a.c.framework.state.TestConnectionStateManager#tes…
martin-g Feb 25, 2022
7b1737b
CURATOR-633: Try to trick javac to not inline the constants' values
martin-g Feb 25, 2022
84ac20d
CURATOR-633: Use reflection to get the actual version of Zookeeper
martin-g Feb 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion ...ramework/src/test/java/org/apache/curator/framework/state/TestConnectionStateManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assumptions.assumeTrue;

import com.google.common.collect.Queues;
import org.apache.curator.framework.CuratorFramework;
Expand Down Expand Up @@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}

@Test
public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's not logically incompatible.

  1. If you remove @Test above, this test doesn't actually run in any condition.
  2. The original error report on JIRA looks because ZOOKEEPER-3269 - Add queueEvent to the Testable facade zookeeper#799 wasn't include in zk 3.5. And the usage of queueEvent here can be emulated by bringing back bridges of queueEvent like CURATOR-558 - part 1 of ZK 3.6 updates #344 removed - it's not a logically incompatible.

Copy link
Member

@tisonkun tisonkun Feb 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Another perspective is that whether or not we want to test that it's zk35compatible. If we don't intend to do that, we can move the test to another (new) class and assign it to zk36 group.

Copy link
Member Author

@martin-g martin-g Feb 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  1. If you remove @Test above, this test doesn't actually run in any condition.

I am not sure why do you explain this to me. I didn't touch @Test and the test is still executed in curator-framework Maven module, where Zookeeper is 3.6.3.
The problem is when this test is executed in curator-test-zk35 module (Zookeeper 3.5.7) where #queueEvent() is not available.
https://issues.apache.org/jira/browse/ZOOKEEPER-3269 is "Fixed" only for 3.6.0+

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @tisonkun you are suggesting to create a set of tests that run only against zk 3.6+
That is to generalise the approach of this fix.

Probably we can create a annotation a little junit extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW if we don't have much time we can keep this patch an unblock ci and the upcoming release

Copy link
Member

Choose a reason for hiding this comment

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

to create a set of tests that run only against zk 3.6+

@eolivelli we have already implemented this. If you want to merge a workaround for unblocking release, it's OK to me, and I'll prepare a followup for explaining.

Copy link
Member

Choose a reason for hiding this comment

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

FYI #411.

@martin-g you can integrate in this PR and close #411.

final int major = getVersionPart("MAJOR");
final int minor = getVersionPart("MINOR");
assumeTrue(major == 3 && minor >= 6 || major > 4, "Zookeeper version must be 3.6 or higher");
Copy link
Member

Choose a reason for hiding this comment

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

Also, the boolean condition should not be ==3 || > 4, what if ==4?

Timing2 timing = new Timing2();
CuratorFramework client = CuratorFrameworkFactory.builder()
.connectString(server.getConnectString())
Expand Down Expand Up @@ -128,4 +132,13 @@ public String getPath() {
CloseableUtils.closeQuietly(client);
}
}

private int getVersionPart(String part) {
try {
return Class.forName("org.apache.zookeeper.version.Info").getDeclaredField(part).getInt(null);
} catch (Exception e) {
e.printStackTrace();
return -1;
}
}
}