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

Removing assertion in asynchronous operation to avoid flaky test #3683

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

mkl201
Copy link

@mkl201 mkl201 commented Jan 20, 2022

What is the purpose of this PR

  • This PR fixes a flaky test to avoid an assertion on an asynchronous operation retrofit2.adapter.rxjava.CancelDisposeTest#cancelDoesNotDispose

Reproduce the test failure

  • The test fails non-deterministically when execution delays occur. To observe a test failure, the unmodified test can be re-executed many times. the likelihood of observing a test failure increases when the test executes on a slow machine. To deterministically reproduce the test failure, introduce an execution delay:
@Test
public void cancelDoesNotDispose() throws Exception{
    Subscription subscription = service.go().subscribe();
    List<Callcalls = client.dispatcher().runningCalls();
    assertEquals(1, calls.size());
    calls.get(0).cancel();
    Thread.sleep(30);
    assertFalse(subscription.isUnsubscribed()); //fail
}

Expected result:

  • The test should run successfully in all runs.

Actual result:

  • In 47 out of 100 runs, we get the failure:
java.lang.AssertionError

Why the test fails

  • Our understanding is that the last assertion of this test is inappropriate. (We suggest alternatives below.) This test case subscribes to receive notifications (line Subscription subscription = service.go().subscribe();). Later, the test requests a cancellation of that subscription (line calls.get(0).cancel();). The problem is that this cancellation is an asynchronous operation; the cancellation does not take effect immediately. Developers certainly did not realize the problem because, as of now, there is no pause between the cancellation statement and the final assertion (line assertFalse(subscription.isUnsubscribed());) passes. But note that the assertion states that the subscription is valid (false that it is _un_subscribed). That only holds because the main thread that runs the test is faster than the thread that runs the cancellation process.

Fix

  • Add a timeout for the test;
  • keep checking whether the subscription is unsubscribed.

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.

1 participant