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

GEODE-6070: Fix ShutdownCommandOverHttpDUnitTest flakiness #5162

Merged

Conversation

kirklund
Copy link
Contributor

Use IgnoredException for DistributedSystemDisconnectedException.

Use IgnoredException for DistributedSystemDisconnectedException.
await().untilAsserted(() -> assertThat(cache.getDistributedSystem().isConnected()).isFalse());
await().untilAsserted(() -> {
assertThat(cache.getDistributedSystem().isConnected())
.as("cache.getDistributedSystem().isConnected()")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it preferable to use withFailMessage than as for this case?

Copy link
Contributor Author

@kirklund kirklund May 28, 2020

Choose a reason for hiding this comment

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

I use as to indicate an alias for the subject of the assertion, and withFailMessage as a full replacement of the failure message.

distributed system isConnected is probably a better example of English for an as in this case. It's kind of interesting to compare the output when using as vs withFailMessage.

Using as:

org.junit.ComparisonFailure: [cache.getDistributedSystem().isConnected()] 
Expected :false
Actual   :true
 <Click to see difference>


	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at org.apache.geode.test.awaitility.WithFailMessageTest.foo(WithFailMessageTest.java:79)

Using withFailMessage:

java.lang.AssertionError: cache.getDistributedSystem().isConnected()

	at org.apache.geode.test.awaitility.WithFailMessageTest.foo(WithFailMessageTest.java:79)

So I only use withFailMessage if I want to actually suppress and replace the native assertion failure message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this is the same assertion failure without as or withFailMessage:

org.junit.ComparisonFailure: 
Expected :false
Actual   :true
 <Click to see difference>


	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at org.apache.geode.test.awaitility.WithFailMessageTest.foo(WithFailMessageTest.java:78)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the explanation and examples!

@kirklund kirklund merged commit 57cc3c7 into apache:develop May 29, 2020
@kirklund kirklund deleted the GEODE-6070-ShutdownCommandOverHttpDUnitTest branch May 29, 2020 16:41
agingade pushed a commit to agingade/geode that referenced this pull request Jun 3, 2020
Use IgnoredException for DistributedSystemDisconnectedException.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants