@awittha could you please review this when you have a chance?

I am not sold on this idea. In general I do not want to special-case any pipeline steps unless

1. It's very difficult or complex for end-users to mock up the correct behavior in their own test-suites
2. The correct special-case behavior is unambiguous.

parallel() is a good example - it's hard to mock up correctly, and I feel confident that everyone will want the parallel streams to try to execute so that they can be tested.

retry() is not so cut-and-dried.

My main objection is running the retry loop the maximum amount of times. This will only happen "in real life" if the code inside the retry loop consistently fails. This does not mimic a situation where there are no problems (and retry only runs once), or where there's an intermittent problem and retry completes before reaching the maximum and allowing the failure to bubble up.

There are three possible correct quantities to run a retry(N) loop:

• 1 time
• 1 < x < N times
• N times

Because of this, I can't say with confidence what the one right behavior to bake into the special case actually is. So, I don't want to ship one behavior that will be wrong for 2/3 of the test situations!

Why wouldn't folks just choose the right behavior and mock up retry() themselves? e.g.

def "all attempts fail"() {
	setup:
		getPipelineMock("retry")(_ as int, _ as Closure) >> { _args ->
			def arguments = _args[0]
			def max = arguments[0]
			def body = arguments[1]
			
			for(int i = 0; i < max; i++) {
				body()
			}
			
			// simulate failure
			throw new IDontExactlyRememberTheRightException()
		}
	when:
		someScript.run()
		// has a retry(3)
	then:
		3 * getPipelineMock("someStepInsideTheRetry")()
}
		
def "intermittent failure overcome by retry"() {
	setup:
		getPipelineMock("retry")(_ as int, _ as Closure) >> { _args ->
			def arguments = _args[0]
			def max = arguments[0]
			def body = arguments[1]
	
			for(int i = 0; i < max - 1; i++) {
				body()
			}
		}
	when:
		someScript.run()
		// has a retry(3)
	then:
		2 * getPipelineMock("someStepInsideTheRetry")()
}

def "no problems in retry"() {
	setup:
		getPipelineMock("retry")(_ as int, _ as Closure) >> { _args ->
			def arguments = _args[0]
			def body = arguments[1]
	
			body()
		}
	when:
		someScript.run()
		// has a retry(3)
	then:
		1 * getPipelineMock("someStepInsideTheRetry")()
}

Well, they can't right now, because jenkins-spock already special-cases any step whose last argument is a Closure, to run that closure exactly once.

So, you can test the "retry runs w/out any problem" case, but it's much more confusing to wire up a "intermittent failure eventually is overcome" situation, and it is impossible (I think) to test the "all retry attempts failed" case.

I think that jenkins-spock should change so that the set of steps that get special-casing of "execute the trailing closure" is configurable. (default: all, to match current behavior and most use-cases) Additionally, a real Documentation site (not just hosted GroovyDoc) is needed to lay out examples like this. There isn't really a single "right" answer for retry() that can be baked into the framework, but common patterns can at least be documented somewhere for folks to pull from.

(withCredentials and withEnv also would benefit from such added flexibility, as the hardcoded special case blocks test authors from actually setting any environment variables inside the body block of those steps)

As it stands, I do not think that adding a special case to always run the body of retry() the maximum number of times without throwing an exception at the end, is a sensible, universal default behavior.

Additionally, parallel, the only other step special-cased by name, is part of the Groovy Pipeline engine - you will always have that step available if you are testing Jenkins pipeline code. retry comes from a separate plugin, and it might not even exist in someone's project! I have a hard time stomaching hard-coded special cases for steps that might not even be there! The way this is implemented, I think that retry(..) would now start "working" even if someone didn't add the Jenkins plugin that contained retry (Pipeline: Basic Steps) to their project's dependency specification. That's objectively incorrect behavior!

In summary:

1. There is not one single correct behavior for retry(N) { ... } in every case, so hardcoding a single special case in jenkins-spock will always be wrong for someone.
2. jenkins-spock's existing blanket special-case for steps that take body blocks, prevents test authors from solving the problem in their own code.
3. the implementation in this PR would allow retry() to succeed even if the Plugin it came from wasn't added to the project.

I believe Jenkins-Spock needs a different solution for retry(), and to "steps that take body blocks" in general.

1. There is not one single correct behavior for retry(N) { ... } in every case, so hardcoding a single special case in jenkins-spock will always be wrong for someone.

I don't agree with this. The only correct behaviour is to retry the body up to N times if it fails. Other the retry step is just being ignored by jenkins-spock.

3. the implementation in this PR would allow retry() to succeed even if the Plugin it came from wasn't added to the project.

I don't understand what you mean by this. Are you referring to user implemented retry steps via pipeline global variables? Or are you concerned that someone could use the retry in their code under test without having a dependency on the workflow-basic-step plugin? That scenario seems unlikely to me.

I am happy with the ability to mock up the right behaviour for retry() on my own. I tried to do just that before creating this PR and ran into the same obstacle you've described.