Multiple finders

[sparkoo]

This adds possibility to set multiple text finders to one build. Last one wins.
It's done in backward compatible way so no current configuration will be affected. I've kept support for pipelines. This patch replaces PR #9.

some implementation details:

• I've replaced setters and set all fields final to keep it safer
• Instead of fields in TextFinderPublisher there is new TextFinderModel which is simple data object with all fields.
• TextFinderPublisher have one TextFinderModel for base configuration and is providing it's values via "fake" getters. Other text finders are stored in list.
• first base text finder is executed, then it loops through list of other finders

pipelines config looks like this:

node {
    echo 'foobar'
}

node {
    findText regexp: 'foobar', alsoCheckConsoleOutput: true, textFinders: [
        finder(regexp: 'foobar', unstableIfFound: true, alsoCheckConsoleOutput: true),
        finder(regexp: 'foobar', notBuiltIfFound: true, alsoCheckConsoleOutput: true)]
}

[sparkoo]

@basil this reimplements #9 which had many conflicts with current master. Can you please review this? Thanks!

[sparkoo]

@basil ping. Can you please review this?

[basil]

Hey @sparkoo, sorry for the delay in reviewing this. Looking at the new pipeline syntax you are proposing, it seems a bit strange that the first text finder uses a different syntax from the subsequent text finders. I also don't think it's obvious at first glance that the last finder will win.

To help me understand this change, would you mind backing up and explaining the high-level problem you are trying to solve? This would help me understand how this new functionality would address that problem.

[sparkoo]

Hi @basil,
first finder is there mostly to keep backward compatibility so current configuration keep working. If you want to add more finders, you create list of other finders inside the first one.
We're using text finder in some of our test jobs to set job status to unstable when some test fails. Recently we've added another check before the testsuite even run and if that extra check fails, we set the job to not-build, to keep jenkins history lucid.
The thing that last finder wins is just way how I've implemented it. I can change that.

[sparkoo]

@basil anything else to this? I would happily rebase it as new changes caused merge conflicts again, but I need to know you would merge it then.

[RadekCap]

Hello @basil would it be possible to merge this PR? I would really appretiate it, thank you.

[basil]

I am willing to go through with the multiple finders feature in general, but I want to make sure we get the API right before committing to it. As I mentioned before, the lack of symmetry between the first finder and the rest still seems off to me, and the way the last finder "wins" also seems non-obvious. I'm struggling to understand the use cases that motivate this design; some examples of how this would be used would be very helpful for me.

Is there any reason one can't use multiple findText invocations with the existing syntax? I was going to try this out myself to see if it was possible, but I haven't had time. If so, that seems preferable to the current proposed API. If not, maybe we can consider adding a new multi-finder publisher with first-class support for multiple finders in its API rather than taping things onto the existing API (which was never designed to support multiple finders).

[sparkoo]

@basil I probably understand where you're going. I'm not using pipelines so I can't add text finder multiple times in job configuration UI. In pipelines, there should be possible to do something like this:

node {
    echo 'foobar'
}
node {
    findText regexp: 'foobar', alsoCheckConsoleOutput: true
    findText regexp: 'foobar', alsoCheckConsoleOutput: true, unstableIfFound: true
    findText regexp: 'foobar', alsoCheckConsoleOutput: true, notBuiltIfFound: true
}

Is that what you mean? I'll try that. However, I'm afraid that list of other finders will have to stay there for UI configuration.

[sparkoo]

@basil Yep, that works just fine. See latest commit 0dd8e1b I've kept the tests to test that. Proposed API is then as in my previous comment. Are you ok with that? Can I rebase?

[sparkoo]

@basil rebased. it's ok to go.

[RadekCap]

@basil - can you merge it please?

[basil]

Thanks for the updates, @sparkoo! This is looking much better on the Pipeline side. Thanks for adding the multiple finder test for Pipeline. I added some code cleanup and did a merge with master to help speed things along.

On the Freestyle side, I have three outstanding concerns at present:

1. We need testing that adding multiple finders works with the UI. We didn't have any existing UI testing before this change, so I added (and merged in) a new TextFinderPublisherFreestyleTest#createTextFinderViaWebClient test. Can you please update this test (or add a new test) to cover adding multiple Text Finders for a Freestyle job via the UI?
2. I want to see an example of how the multiple finder support will work in Freestyle jobs when the UI is not used; i.e., when the configuration is done with code via Job DSL. Such a setup should be possible via the Dynamic DSL, which you can access via http://${JENKINS_URL}/plugin/job-dsl/api-viewer/index.html after installing the Job DSL plugin. I deployed your changes to a test Jenkins server and saw the new textFinderModel present in the Dynamic DSL, but it's not clear to me how it would be used yet. Can you provide an example of the usage? Then I can review the API as well as prepare to include it in the documentation once this is released.
3. Per the Constructor vs. setters section of the Jenkins documentation for writing Pipeline-compatible plugins: "It is a good idea to replace a lengthy @DataBoundConstructor with a short one taking just truly mandatory parameters (such as a server location). For all optional parameters, create a public setter marked with @DataBoundSetter (with any non-null default value set in the constructor or field initializer)." When I added Pipeline support in [JENKINS-34983] - Add pipeline support #12, I did exactly that by marking the older multi-argument constructor as @Deprecated and adding a new single-argument @DataBoundConstructor. Your change regresses my previous change by once again going back to a multi-argument constructor annotated with @DataBoundConstructor. Please use a single-argument constructor annotated with @DataBoundConstructor. If the Freestyle UI needs to use a multi-argument constructor, it should use one annotated with @Deprecated and without @DataBoundConstructor. Also, please restore the @DataBoundSetter-annotated setters for the various fields. They should delegate to setters in the primary text finder (which will need to become mutable to support this). It should be possible to implement all of this in a way that preserves Java-level compatibility; i.e., you should revert your changes to existing tests in TextFinderPublisherFreestyleTest, and those tests should still compile.

[basil]

@basil - can you merge it please?

@RadoslavCap In order to merge this, I need to be confident in the API, test coverage, and implementation. At this time, I have (minor!) outstanding concerns in each of those three areas, but once those concerns are addressed I'll be happy to move forward with merging this.

[basil]

Another issue I noticed while doing manual UI testing: after you click on the "Add additional Text Finder" button, it creates a drop-down with "Text Finder" which you have to click on again to get the additional finder to show up in the UI. This seems suboptimal in that it requires two clicks. An example of the correct behavior would be how the HTML Publisher plugin does it. It just has one "Add" button, and after one click the additional publisher is added to the UI.

[sparkoo]

closing this one in favor of #47