Skip to content

Rehydrate components with thisObject pointing to the script#304

Closed
Willem1987 wants to merge 1 commit intojenkinsci:masterfrom
Willem1987:change-this-reference-to-script
Closed

Rehydrate components with thisObject pointing to the script#304
Willem1987 wants to merge 1 commit intojenkinsci:masterfrom
Willem1987:change-this-reference-to-script

Conversation

@Willem1987
Copy link
Contributor

Attempt to fix #303

@Willem1987 Willem1987 force-pushed the change-this-reference-to-script branch from a6df9ae to 722d5b8 Compare October 10, 2020 18:42
@Willem1987
Copy link
Contributor Author

Unfortunately this works locally. Any idea how to reproduce?

@nre-ableton
Copy link
Contributor

It fails locally for me. Here's the stacktrace I get from running the unit tests in IntelliJ:

groovy.util.ResourceException: Cannot open URL: file:/home/nre/Code/nre-ableton/JenkinsPipelineUnit/src/test/jenkins/jenkinsfiles/Declarative_This_Jenkinsfile

	at groovy.util.GroovyScriptEngine.getResourceConnection(GroovyScriptEngine.java:411)
	at groovy.util.GroovyScriptEngine.loadScriptByName(GroovyScriptEngine.java:556)
	at groovy.util.GroovyScriptEngine$loadScriptByName$0.call(Unknown Source)
	at com.lesfurets.jenkins.unit.PipelineTestHelper.loadScript(PipelineTestHelper.groovy:399)
	at com.lesfurets.jenkins.unit.PipelineTestHelper$loadScript$5.callCurrent(Unknown Source)
	at com.lesfurets.jenkins.unit.PipelineTestHelper.runScript(PipelineTestHelper.groovy:413)
	at com.lesfurets.jenkins.unit.PipelineTestHelper$runScript$4.call(Unknown Source)
	at com.lesfurets.jenkins.unit.BasePipelineTest.runScript(BasePipelineTest.groovy:338)
	at com.lesfurets.jenkins.unit.BasePipelineTest$runScript.callCurrent(Unknown Source)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:52)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:154)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:166)
	at com.lesfurets.jenkins.unit.declarative.TestDeclarativePipeline.jenkinsfile_this(TestDeclarativePipeline.groovy:27)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
	at org.junit.runners.Suite.runChild(Suite.java:127)
	at org.junit.runners.Suite.runChild(Suite.java:26)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:160)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:220)
	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:53)

I would suggest making a fresh checkout with this branch or running git clean -dffx and then re-running the unit tests. Also, it might help to rebase this branch on upstream master, since there have been some other PRs merged since this one was created.

@Willem1987 Willem1987 force-pushed the change-this-reference-to-script branch 2 times, most recently from c44f199 to e757d53 Compare October 14, 2020 17:38
@Willem1987
Copy link
Contributor Author

I have overwritten my fork of master with upstream master and re-applied commit to that. Then i force pushed the state to the existing branch. I tried the git clean -dffx but locally i could not get it to break.
I hope the up to date master with the cherry picked commit will help.

@Willem1987
Copy link
Contributor Author

The appveyor seems to run the test and work? Could it be a jdk thing or specific environment?

I tried this test on a windows (virtual workstation) and macbook.

@Willem1987
Copy link
Contributor Author

I changed from oracle jdk 8 to openjdk 8 which i thought to have seen in the travis build.

openjdk version "1.8.0_265"
OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_265-b01)
OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.265-b01, mixed mode)

@Willem1987 Willem1987 force-pushed the change-this-reference-to-script branch from e757d53 to bf70041 Compare October 14, 2020 18:40
@Willem1987
Copy link
Contributor Author

Okay now im lost. I renamed the jenkinsfile to Declarative_thisObject_JenkinsFile and the test to jenkinsfile_thisObject and now it seems to work? So somehow this in a filename might cause problems? Or something else fixed it

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.

Please improve tests. assertJobStatusSucces() is not enough IMHO

@Willem1987 Willem1987 force-pushed the change-this-reference-to-script branch 2 times, most recently from e12cdd3 to 4b7ab37 Compare October 16, 2020 14:35
@Willem1987 Willem1987 requested a review from stchar October 16, 2020 19:42
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.

Could you please add few more changes in tests and I would be glad to merge it


String testThis(Script scriptRef, String text){
echo "This is a script?: ${this == scriptRef}"
return text
Copy link
Contributor

Choose a reason for hiding this comment

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

return "$text: This is a script?: ${this == scriptRef}"

Would allow to:
assertCallStack().contains("pipeline unit tests completed: This is a script?: true")

So you can help other contributors to detect broken places in the code in later updates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we could only call the method in 1 place. I called it from environment, options, when, trigger and script content now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scratch that, it technically works, but the values returned would not be valid in real life. For test it is okay i think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated it a little to pass in Object. So that this always works and we can check instanceof

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to that i added an assert so that it may not contain false

@Willem1987 Willem1987 force-pushed the change-this-reference-to-script branch 6 times, most recently from eb26bee to cb81df0 Compare October 17, 2020 12:33
…l make this.sh work and make this in the pipeline script passable to methods.
@Willem1987 Willem1987 force-pushed the change-this-reference-to-script branch from cb81df0 to dcbddce Compare October 17, 2020 13:03
@Willem1987 Willem1987 requested a review from stchar October 19, 2020 12:40
@Willem1987
Copy link
Contributor Author

@nre-ableton @stchar Is there anything i can do on this? I updated the MR according to comments i think. If i need to do anything let me know. I started using jenkinsPipelineUnit in our builds and would like the this ref feature.

@nre-ableton
Copy link
Contributor

I'll let @stchar take things from here, since he's been the primary reviewer on this PR.

@Willem1987
Copy link
Contributor Author

Willem1987 commented May 2, 2021

I'll let @stchar take things from here, since he's been the primary reviewer on this PR.

@nre-ableton Though i understand this viewpoint, it has been months now. Is he still active? Is there anything i can do to further progress this?

@nre-ableton
Copy link
Contributor

I'll let @stchar take things from here, since he's been the primary reviewer on this PR.

@nre-ableton Though i understand this viewpoint, it has been months now. Is he still active? Is there anything i can do to further progress this?

My understanding is that he's recently become a father, so I expect he's pretty busy (and also possibly on paternity leave). Probably doesn't hurt to ping him again (hi @stchar!), but I don't know when he'll be back.

@Willem1987
Copy link
Contributor Author

Closing due too revised insights from #299

@Willem1987 Willem1987 closed this Jun 7, 2021
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.

thisObject inside pipeline script referes to Declaration class

3 participants