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

fix(declarative) withCredentails usernamePassword mocking conflict #311

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

stchar
Copy link
Contributor

@stchar stchar commented Oct 16, 2020

Fixes #310

Copy link
Contributor

@nre-ableton nre-ableton left a comment

Choose a reason for hiding this comment

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

Nice fix!

@nre-ableton nre-ableton merged commit 19dc388 into master Oct 16, 2020
@Willem1987
Copy link
Contributor

Question:

Why would we add a param in this case? The ParametersDeclaration has the string method when calling within a parameters closure.

When the string is called on the script outside the closure, i would not expect params to be added. That's why i did not go through the interceptor. Though i'm not sure if thats building on from #299

I see a clear difference between the string method being called as a param to a downstream build vs the string method inside the parametersdeclaration. I feel only the parametersdeclaration should actually add params.

@stchar
Copy link
Contributor Author

stchar commented Oct 16, 2020

@Willem1987 I cannot determine whether the string mock is called in which context (parameters {} or withCredentials([...]) {...} ) that's why I've implemented it as is

@Willem1987
Copy link
Contributor

@stchar If i'm correct the call within the Parameters block should end up in ParametersDeclaration class first before getting to the outer lying script. But i could be mistaken. The tests seemed to work the way i tried to implement it

@stchar
Copy link
Contributor Author

stchar commented Oct 17, 2020

@Willem1987
It looks like parameters block should have it's own string interceptor or to be updated to support interceptor for withCredentials:

registerAllowedMethod('string', [Map]) acts differently in DeclarativePipelineTest and BasePipelineTest
But we hadn't seen any differences until withCredentials has been refactored in BasePipelineTest.

DeclarativePipelineTest inherits BasePipelineTest so withCredentials interceptor had been inherited
in DeclaratievePipelineTest which fired errors in my tests once I updated version of JPU up to 1.8

I would prefer ParametersDeclaration to shadow pipeline dsl but not to mock any pipeline plugins callbacks

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.

withCredentials doesn't work with declarative pipeline after 1.8
3 participants