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

Fix RetryRule #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

alb-i986
Copy link

  1. fix NPE

The following unit test shows the bug:

public class RetryRuleTest {

    @Test
    public void asd() throws Throwable {
        RetryRule sut = new RetryRule(1);

        Description desc = Description.createTestDescription("class", "name");
        Statement failingTest = new Fail(new ExpectedException("simulated failure"));

        try {
            sut.apply(failingTest, desc).evaluate();
        } catch (ExpectedException e) {
            // expected
        }
    }

    private static class ExpectedException extends RuntimeException {
        public ExpectedException(String message) {
            super(message);
        }
    }
}
  1. refactor: no need to use AtomicInteger

alb-i986 added 2 commits May 30, 2016 15:54
Declare retryCount as final, and use a local variable instead.
caughtThrowable could have been not initialized.
E.g. if retryCount is 1, and the first execution failed, it would have thrown null.
@ndmanvar
Copy link
Contributor

@mehmetg Could you please review?

try {
base.evaluate();
return;
} catch (Throwable t) {
if (retryCount.get() > 0 && description.getAnnotation(Retry.class)!= null) {
Copy link
Contributor

@mehmetg mehmetg Jun 2, 2016

Choose a reason for hiding this comment

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

Wouldn't adding a "=" to the first comparison fix or pulling out the assignment to caughtThrowable from within the if statement? Just like the refactored code does.

@mehmetg
Copy link
Contributor

mehmetg commented Jun 2, 2016

The point is valid, just don't see a reason for the rewrite this fix can be handled, with a single line change.
AtomicInteger use is not required, but this is not a real concern here.

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.

3 participants