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

Add declarative pipeline harness #133

Closed

Conversation

dgoetsch
Copy link

No description provided.

@dgoetsch
Copy link
Author

dgoetsch commented Feb 10, 2019

example usage:

class HarnessTest extends BasePipelineTest {

    String jenkinsfileName = "Jenkinsfile"
    Script jenkinsfile = null

    DeclarativePipelineHarness harness

    def loadJenkinsfile() {
        this.jenkinsfile = super.loadScript(jenkinsfileName)
    }


    @Before
    void setup() {
        if(harness == null || jenkinsfile == null) {
            super.setUp()
            harness = new DeclarativePipelineHarness(this)
            loadJenkinsfile()
        } else {
            harness.clearAllCalls()
        }
    }

    @Test
    void smokeTest() {
        jenkinsfile.run()
        harness.runCall('pipeline')
        harness.runCall('stages')
        harness.runIdentified('stage', 'Build')
        harness.runCall('steps')
        def call = helper.callStack.find { it.methodName == 'sh' }
        assertThat(call.args.toList(), equalTo(['gradle build']))
    }
}

Before I formalized this unit test, I would like to get feedback. Once I am granted tentative approval, I will add a unit test.

Also, Should I have a DeclarativeJenkinsfileTest class, ala BaseJenkinsfileTest (probablly extending BaseJenkinsfileTest)?

@dgoetsch
Copy link
Author

dgoetsch commented Feb 10, 2019

This approach is very different from https://github.com/jenkinsci/JenkinsPipelineUnit/pull/13/files, in that all scopes are not implicitly executed. It forces the user to define which part of the pipeline is being executed, and allows stricter control and verification for complex pipelines.

This has the effect of reducing the coverage of any individual test; which decouples dependencies across disparate blocks (really helps with testing complex pipelines, i'm working with a pipeline with 40+ stages). This will make tests more maintainable and reduce the number of tests that would need to be changed when the pipeline is changed.

Copy link
Contributor

@stchar stchar left a comment

Choose a reason for hiding this comment

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

Hi, coluld you please add some tests, I would be glad to release it. But I need some to make sure your functionality wouldn't be broken in advance.

@stchar stchar added this to the 1.1 milestone Mar 7, 2019
@dgoetsch
Copy link
Author

@stchar of course. I will update this PR when I have some time

@dgoetsch dgoetsch force-pushed the feature/declarative-pipeline-support branch from cbb46de to a37fd82 Compare March 25, 2019 19:22
@dgoetsch
Copy link
Author

@stchar I finally got around to updating this. I've added a unit test and completed the implementation, as well as added a bit of documentation to the README.

I didn't not add a unit test that tests libraries, could do that if desired. I dont' know if it would be a valuable test since the implementations are properly isolated. I have used the same code with libraries and it uses the same mechanism to run as the other tests, just adds some structure around the steps. From an implementation standpoint, the library loading is unaffected by the usage of the BaseDeclarativePipelineTest

@dgoetsch
Copy link
Author

dgoetsch commented Apr 2, 2019

Ooof. I just realized I did my most recent commit on a different machine and it didn't recognize my git user

@dgoetsch dgoetsch force-pushed the feature/declarative-pipeline-support branch from a37fd82 to 51efd5f Compare April 3, 2019 15:38
@dgoetsch
Copy link
Author

dgoetsch commented Apr 3, 2019

@stchar is there anything else that you need from me before this can be merged?

@dgoetsch dgoetsch force-pushed the feature/declarative-pipeline-support branch from 51efd5f to cd84f14 Compare April 3, 2019 15:46
super.jenkinsfile.run()
harness
.runStage('init')
.runClosureStep('steps')
Copy link

@AlanFoster AlanFoster Apr 4, 2019

Choose a reason for hiding this comment

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

@dgoetsch This looks cool 👍

Is there a way to just run an entire declarative pipeline in a similar fashion to Jenkins too?

harness.run()

Copy link
Author

Choose a reason for hiding this comment

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

There is not a way to do that at this point.

Copy link
Author

Choose a reason for hiding this comment

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

I believe this is relevant to the comment from @stchar asking about built in support for parameters / environment / pre / post.

}
}

class ParameterizedCallStack<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

ParameterizedCallStack and CallStack looks almost the same,
i'm just wondering if these classes could be coupled by inheritance e.g.

class ParameterizedCallStack extends CallStack

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe replace it with just one class,
As result you can reduce the logic splits you need to handle parameterized or non-parameterized closures.

Copy link
Author

Choose a reason for hiding this comment

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

Superficially, these classes do look similar. I don't think this change provides a lot of value or is straightforward. Every method implementation is different, and the critical methods have different signatures.

Parameterized call stack has a lot of extra code to deal with the parameter for the closure step, and the important methods have subtly different signatures.

void run(Integer idx = 0, doClear = true) (CallStack - there is nothing to return)
vs
T run(Integer idx = 0, doClear = true) (ParameterizedCallStack - must return parameter so it can be validated)
and this method is only in ParameterizedCallStack: void run(T value, doClear = true)

If I merge this, I have to handle these differences via if statements, rather than by having different classes define behavior. This would be complicated and messy.

I don't think we gain anything from introducing inheritance or merging these classes.

Finally, these aren't meant to be used by anyone, if groovy was able to respect private classes, these would be marked as private.

@stchar
Copy link
Contributor

stchar commented Apr 4, 2019

@dgoetsch, I took a closer look on your implementation, it looks cool, it allows to test declarative pipeline running it stage by stage.
it also has some lack of support of some essential features (imho) of declarative pipeline like:

  • parameters {}
  • pre {}
  • post {}
  • environment {}
    There was an attempt to implement so in Support declarative pipeline syntax #13 (which you mentioned)
    which already has implementation of these features of declarative pipeline.

Maybe we can try to combine your solution with Ozan's. What do you think?

Copy link
Author

@dgoetsch dgoetsch left a comment

Choose a reason for hiding this comment

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

@stchar I'm not sure. I believe such an implementation would be of a different scope. This is really intended to be a true unit test - mock out everything and test very focuses parts of the code with the environment set up in a predictable and straightforward way. I've implemented parameters and environment variables by binding map variables. The pre + post blocks are validated by invoking them like any other closure. I had chosen not to treat any of the closure steps as special.

I'm not trying to run things like jenkins is . or to assert jenkins behavior, I'm trying to assert that the code I wrote behaves the way I expect it to, and is structured as I expect it to be structured.

Such a test, as you describe it, could be valuable for relatively simple pipelines, but I think that would be a different harness. In my tests, I had unit tests that validated the parameters/pre/post/environment block contents.

It is possible to add variable injection for parameters and environment as mixins that could be called in a before step, something like "AutomaticParameters(this)" - which would override the registration for the parameters block and automatically bind values in the parameters block to a variable. At the moment I can't think of what features we would add for pre/post unless the entire pipeline was being invoked.

}
}

class ParameterizedCallStack<T> {
Copy link
Author

Choose a reason for hiding this comment

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

Superficially, these classes do look similar. I don't think this change provides a lot of value or is straightforward. Every method implementation is different, and the critical methods have different signatures.

Parameterized call stack has a lot of extra code to deal with the parameter for the closure step, and the important methods have subtly different signatures.

void run(Integer idx = 0, doClear = true) (CallStack - there is nothing to return)
vs
T run(Integer idx = 0, doClear = true) (ParameterizedCallStack - must return parameter so it can be validated)
and this method is only in ParameterizedCallStack: void run(T value, doClear = true)

If I merge this, I have to handle these differences via if statements, rather than by having different classes define behavior. This would be complicated and messy.

I don't think we gain anything from introducing inheritance or merging these classes.

Finally, these aren't meant to be used by anyone, if groovy was able to respect private classes, these would be marked as private.

super.jenkinsfile.run()
harness
.runStage('init')
.runClosureStep('steps')
Copy link
Author

Choose a reason for hiding this comment

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

There is not a way to do that at this point.

@dgoetsch
Copy link
Author

dgoetsch commented Apr 4, 2019

I'll wait until we resolve conversations to fix the readme conflicts

@stchar stchar removed this from the 1.2 milestone Dec 8, 2019
@stchar
Copy link
Contributor

stchar commented Jan 29, 2020

Sorry to say but this PR has lack of features, and needs significant rework IMHO.
I'm canceling it in favour of #13

@stchar stchar closed this Jan 29, 2020
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.

3 participants