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

Make @Retry repeatable (#1030) #1197

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

Conversation

Vampire
Copy link
Member

@Vampire Vampire commented Jul 9, 2020

Make @Retry repeatable (#1030)


This change is Reviewable

for different situations:

[source,groovy]
----
Copy link
Member

Choose a reason for hiding this comment

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

we want code backed docs where ever possible

Copy link
Member Author

Choose a reason for hiding this comment

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

All pimped

@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #1197 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1197   +/-   ##
=========================================
  Coverage     75.96%   75.96%           
  Complexity     3647     3647           
=========================================
  Files           393      393           
  Lines         11113    11113           
  Branches       1369     1369           
=========================================
  Hits           8442     8442           
  Misses         2192     2192           
  Partials        479      479           
Impacted Files Coverage Δ Complexity Δ
spock-core/src/main/java/spock/lang/Retry.java 100.00% <ø> (ø) 0.00 <0.00> (ø)
...work/runtime/extension/builtin/RetryExtension.java 93.33% <100.00%> (ø) 12.00 <2.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3aefd7...1bd9edb. Read the comment docs.

@Vampire Vampire force-pushed the repeatable-retry-annotation branch 4 times, most recently from d2e2b5e to 3b4d2e3 Compare July 13, 2020 19:42
@Vampire Vampire force-pushed the repeatable-retry-annotation branch from 3b4d2e3 to 1bd9edb Compare July 14, 2020 14:50
def sql = Sql.newInstance("jdbc:h2:mem:", "org.h2.Driver")

// tag::example-d[]
@Retry(exceptions = IllegalArgumentException, count = 2)
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a test, that actually alternately throws both these exceptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is unfortunately a good idea, though in the test spec, not the doc spec.
Unfortunately, because I think the changes were too naive for the retry extension logic to properly work in that case in its current state. I have to rework that probably when I'm a bit less sleepy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or you might have a look, as you invented the whole extension and probably know the code better than me.

@@ -44,7 +46,7 @@ private boolean noSubSpecWithRetryAnnotation(SpecInfo spec) {
}

private boolean noRetryAnnotation(NodeInfo node) {
return !node.getReflection().isAnnotationPresent(Retry.class);
return !node.isAnnotationPresent(Retry.class);
Copy link
Member

Choose a reason for hiding this comment

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

What about Retry.Container?

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.

2 participants