CURATOR-633: Fix backward compatibility in tests #409
Conversation
| @@ -92,6 +94,7 @@ public void stateChanged(CuratorFramework client, ConnectionState newState) | |||
|
|
|||
| @Test | |||
| public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception { | |||
| assumeTrue(() -> (getMajor() == 3 && getMinor() >= 6) || (getMajor() > 4), "Zookeeper version must be 3.6 or higher"); | |||
There was a problem hiding this comment.
This does not really work...
It seems javac inlines the values of the constants and the test still prints 3 and 6 even for curator-test-zk35 modole.
|
Yes!
The build fails with JDK 1.8 and 11.
…On Fri, Feb 25, 2022, 18:30 Enrico Olivelli ***@***.***> wrote:
***@***.**** commented on this pull request.
So you mean that this tests didn't work with 3.5?
I can't remember about this problem
@Randgalt <https://github.com/Randgalt> @tisonkun
<https://github.com/tisonkun>
—
Reply to this email directly, view it on GitHub
<#409 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABYUQS5Y6RDA5FJOD4GPW3U46VBVANCNFSM5PKIMVXA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Can you rebase in order to pick your changes? |
…tConnectionStateRecoversFromUnexpectedExpiredConnection only for Zookeeper 3.6.x Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
martin-g
force-pushed
the
curator-633-execute-test-only-on-zookeeper-3.6.x
branch
from
f2056aa to
84ac20d
Compare
|
CI is green! 🥳 |
| @@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState) | |||
| } | |||
|
|
|||
| @Test | |||
| public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception { | |||
| void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception { | |||
There was a problem hiding this comment.
I think it's not logically incompatible.
- If you remove
@Testabove, this test doesn't actually run in any condition. - 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
queueEventhere can be emulated by bringing back bridges ofqueueEventlike CURATOR-558 - part 1 of ZK 3.6 updates #344 removed - it's not a logically incompatible.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- If you remove
@Testabove, 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+
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
BTW if we don't have much time we can keep this patch an unblock ci and the upcoming release
There was a problem hiding this comment.
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.
| @@ -91,7 +92,10 @@ public void stateChanged(CuratorFramework client, ConnectionState newState) | |||
| } | |||
|
|
|||
| @Test | |||
| public void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception { | |||
| void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception { | |||
There was a problem hiding this comment.
BTW if we don't have much time we can keep this patch an unblock ci and the upcoming release
| void testConnectionStateRecoversFromUnexpectedExpiredConnection() throws Exception { | ||
| 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"); |
There was a problem hiding this comment.
Also, the boolean condition should not be ==3 || > 4, what if ==4?
Run TestConnectionStateManager#testConnectionStateRecoversFromUnexpectedExpiredConnection only for Zookeeper 3.6.x