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

Implemented repeat attempts by default #394

Merged
merged 6 commits into from
May 17, 2021

Conversation

danieldisu
Copy link
Contributor

In this PR I've added a method that allows users to set repeat attempts by default, it works the same way than allowFlakyAttemptsByDefault but applies the @Retry annotation to all methods

Sloy
Sloy previously approved these changes May 11, 2021
Copy link
Member

@Sloy Sloy left a comment

Choose a reason for hiding this comment

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

🙌

@rocboronat
Copy link
Member

Hi!

Wow, having a @repeat is great! Just one thing. What do the tests do? I don't get the names: allowFlakyStatementReturnedWhenAllowFlakyAnnotationFoundEvenRepeatStatement and lastStatementReturnedWhenRepeatSetRepeatAttemptsByDefaultAndAllowFlakyStatementAtTheSameTime. In addition, the tests do and assert the same.

Thanks @danieldisu!

alorma
alorma previously approved these changes May 11, 2021
Copy link
Member

@alorma alorma left a comment

Choose a reason for hiding this comment

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

Good!!

@danieldisu danieldisu dismissed stale reviews from alorma and Sloy via c60f9f3 May 12, 2021 06:28
@danieldisu
Copy link
Contributor Author

danieldisu commented May 12, 2021

@rocboronat Both tests were doing the same, I removed one and change the name of the other for lastStatementReturnedWhenRepeatAttemptsByDefaultAndAllowFlakyStatementUsedAtTheSameTime, I think this name it's more clear, but I'm open to suggestions!

In addition, the tests do and assert the same.

I've extracted the Statement building to methods, but I'm not sure that's what you were pointing here 🤔

@danieldisu
Copy link
Contributor Author

@Sloy @rocboronat @alorma Can you review it again 🙏

* Use this method when constructing the Rule to apply a default behavior of @[Repeat] without having to add the annotation to
* each test. This can help you to find flaky tests.
* <br></br>
* The default behavior can be overridden with [Repeat] or [Repeat].
Copy link
Member

Choose a reason for hiding this comment

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

[Repeat] or [Repeat]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was [Repeat] or [AllowFlaky], fixed!

@@ -44,6 +44,16 @@ public void repeatStatementReturnedWhenRepeatAnnotationFound() throws Exception
assertTrue(resultStatement instanceof RepeatStatement);
}

@Test
public void repeatStatementReturnedWhenRepeatSetRepeatAttemptsByDefault() 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.

Sorry, I don't get the repeat set repeat 😅

Maybe repeatStatementReturnedWhenSettingDefaultRepeatAttempts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it make sense 👍 , changed

}

@Test
public void lastStatementReturnedWhenRepeatAttemptsByDefaultAndAllowFlakyStatementUsedAtTheSameTime() 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.

Not that important because it's easier to get, but just to be coherent:

lastStatementReturnedWhenDefaultRepeatAttemptsAndAllowFlakyStatementUsedAtTheSameTime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 👍

@rocboronat
Copy link
Member

There are some comments but if the Adevinta team wants to go on, it's good for me 👍 Thanks @danieldisu!

rocboronat
rocboronat previously approved these changes May 17, 2021
Copy link
Member

@rocboronat rocboronat left a comment

Choose a reason for hiding this comment

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

That's it then! Thanks!

@rocboronat
Copy link
Member

Let's wait for the CI to finish testing...

@alorma alorma merged commit d31b84d into AdevintaSpain:master May 17, 2021
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.

4 participants