Add match-all addShMock invocation

[rmartine-ias]

The existing configuration for a match-everything sh mock uses null as the argument, which causes this error:

groovy.lang.GroovyRuntimeException: Ambiguous method overloading for method com.lesfurets.jenkins.unit.PipelineTestHelper#addShMock.
Cannot resolve which method to invoke for [null, class TestIsDatabricksRepo$_testNoDbxFolder_closure5] due to overlapping prototypes between:
	[class java.lang.String, class groovy.lang.Closure]
	[class java.util.regex.Pattern, class groovy.lang.Closure]

This can be resolved by using a pattern that matches everything (~/(?s).*/). We need the embedded DOTALL0 flag, because otherwise multiline scripts like the following are not caught:

    def status = sh(script: '''\
    echo hi
    ls -d .dbx
    '''.stripIndent(), returnStatus: true)

If there is a better way to do this, let me know! But for now, this fixes the docs bug of the docs suggesting an incorrect configuration.

Testing done

Submitter checklist

Preview Give feedback

1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
2. Ensure that the pull request title represents the desired changelog entry
3. Please describe what you did
4. Link to relevant issues in GitHub or Jira
5. Link to relevant pull requests, esp. upstream and downstream changes
6. Ensure you have provided tests - that demonstrates feature works or fixes the issue

[rmartine-ias]

If tests or more thorough reproduction steps for this change are needed, let me know.

[nre-ableton]

Great find! 👍 I guess we never had a test case for this behavior, and there should have been one, it seems. 😅 So I tried to add such a test:

    @Test()
    void runShWithNull() {
        // given:
        def helper = new PipelineTestHelper()
        helper.addShMock(null) { script ->
            return [stdout: '', exitValue: 2]
        }

        // when:
        def status = helper.runSh(returnStatus: true, script: 'echo foo')

        // then:
        assertThat(status).isEqualTo(2)
    }

...and I ran into the same GroovyRuntimeException for ambiguous method overloading as you did. So it's definitely broken.

However, I don't think that the best solution here is to fix the documentation, but rather we should change the API to better handle catch-all cases. I appreciate that you may have wanted to avoid such a breaking change in your PR, but I do believe it would be the better solution (also considering that the method documentation for all addShMock and addBatMock overrides claim that passing null for the first argument is allowed, when this will fail).

What would you think about adding an override like this?

void addShMock(Closure callback) {
    mockShHandlers << new MockScriptHandler(~/(?s).*/, callback)
}

Which means that users would call helper.addShMock(null) { /* whatever */ }. I realize that this is a big ask for what was initially a rather small PR, so just let me know if you'd rather that I take care of it (when I have some time, that is). Otherwise if you are keen to do so then feel free to rework this PR.

[rmartine-ias]

Taking a crack at this! I have two solutions for you, will push code in a sec.

Your suggested fix still fails, if I'm running it right, because no first param != null first param. So we can either:

1. make the suggested match-all call addShMock() {} with no args
2. import javax.lang.model.type.NullType and define a method that takes NullType and Closure, which is more specific than String or Pattern, so we're good to keep addShMock(null) {}

I'm not sure which is more idiomatic Java, I will ask around. Let me know if you have thoughts.

[rmartine-ias]

Alright, I've pushed two commits, one for each option. The first seems "cleaner" to me, but diverges from the existing API, so people would have to change their test code to update. The second feels a bit more like a hack, but keeps the API the same as it used to be.

Both also don't use a regex to match, but just match all, which I think is a little less error prone.

[nre-ableton]

@rmartine-ias thanks for taking the time to look into this! I agree with you that the first approach (the one in 84c149a) looks nicer, so let's go with it. I'll deal with documenting the breaking change in the release notes etc.

Would you mind reworking your branch to drop the other commits? 🙏 Thanks also for adding tests; once this branch is cleaned up, it should basically be good to go.

[rmartine-ias]

Done & done, that's why I kept the commits separate. Thank you so much!

[nre-ableton]

Thanks @rmartine-ias for putting in the work on this PR! 👍 I'll merge it on Monday and make a release directly afterwards.

[nre-ableton]

I just released https://github.com/jenkinsci/JenkinsPipelineUnit/releases/tag/v1.20 and documented this change in the release notes. Thanks again for your contribution! 👍