-
Notifications
You must be signed in to change notification settings - Fork 99
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 integration tests for both Freestyle and Pipeline jobs #81
Conversation
<configuration> | ||
<warSourceDirectory>../client/target</warSourceDirectory> | ||
</configuration> | ||
</plugin> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is necessary to allow the Swarm Client JAR to be downloaded from integration test context. Normally, this JAR is made available through the .hpi
file via the maven-dependency-plugin
(see lines 25–51 above). However, in integration test context the .hpi
file is ignored and the test uses the .hpl
version of the plugin. As such we need to customize the .hpl
metadata to include the Swarm Client JAR on its classpath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not safe, because it also injects the client's classes to the plugin. And not all this libs are guaranteed to be compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not safe, because it also injects the client's classes to the plugin. And not all this libs are guaranteed to be compatible.
It is safe, for the reasons explained in my top-level reply.
@Test | ||
public void buildShellScriptAfterRestart() throws Exception { | ||
Assume.assumeNotNull( | ||
System.getProperty("port"), "This test requires a fixed port to be available."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests that a Swarm Client launched with -deleteExistingClients
can reconnect to a Jenkins master after the master has been restarted. The -deleteExistingClients
only works in this use case when the Jenkins master has the same port before and after the restart; however, RestartableJenkinsRule
chooses a random port each time. Therefore, we are forced to skip this test unless the tests are being run with -Dport=<port-number>
to fix the port before and after the restart. Unfortunately, this means this test cannot run on ci.jenkins.io
since AFAIK there is no isolation between builds there (so tests cannot assume that a particular port will be free).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RestartableJenkinsRule
chooses a random port each time
That ought to be fixable, just like it already keeps the same $JENKINS_HOME
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll work on that in a downstream PR so as to keep this PR from developing a dependency on a Jenkins test harness change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. If you want CI on that,
mvn -f jenkins-test-harness clean install source:jar-no-fork deploy:deploy -DaltDeploymentRepository=maven.jenkins-ci.org::default::https://repo.jenkins-ci.org/snapshots/ -DskipTests
and specify the timestamped snapshot for jenkins-test-harness.version
. Though I should probably just incrementalify it (JEP-305) to make this easier. Give me a moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. If you want CI on that,
mvn -f jenkins-test-harness clean install source:jar-no-fork deploy:deploy -DaltDeploymentRepository=maven.jenkins-ci.org::default::https://repo.jenkins-ci.org/snapshots/ -DskipTestsand specify the timestamped snapshot for
jenkins-test-harness.version
.
Thanks! I did as you suggested in #83.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @basil ! It would be great to have such tests, but I do not think they justify inclusion of the client code into the plugin. It may cause various issues, e.g. remoting lib conflicts for newer Jenkins versions.
I highly recommend starting an external process or a Docker fixture if you want to test the connection logic
<configuration> | ||
<warSourceDirectory>../client/target</warSourceDirectory> | ||
</configuration> | ||
</plugin> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not safe, because it also injects the client's classes to the plugin. And not all this libs are guaranteed to be compatible.
Thanks for the review, @oleg-nenashev. The client JAR is already included in the plugin as of #62, which made the client JAR available from the master via HTTP at #62 successfully made the client JAR available via HTTP from the
No, because I verified my understanding experimentally both in production context (where the existing logic from #61 applies) and in test context (where my new logic applies) as follows:
Based on the above reasoning and experimental verification, I am confident this is not an issue.
I am already doing the former (starting an external process) in this change. See the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see this! Left some minor comments.
@Test | ||
public void buildShellScriptAfterRestart() throws Exception { | ||
Assume.assumeNotNull( | ||
System.getProperty("port"), "This test requires a fixed port to be available."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RestartableJenkinsRule
chooses a random port each time
That ought to be fixable, just like it already keeps the same $JENKINS_HOME
.
plugin/pom.xml
Outdated
<dependency> | ||
<groupId>org.jenkins-ci.plugins.workflow</groupId> | ||
<artifactId>workflow-support</artifactId> | ||
<version>2.13</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduce a POM property to ensure that the unclassified artifact is also in the same version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW the GH feature of marking a conversation as resolved works very nicely in my experience to hide obsolete discussions.
@ClassRule public static BuildWatcher buildWatcher = new BuildWatcher(); | ||
|
||
private final ProcessDestroyer processDestroyer = new ProcessDestroyer(); | ||
private final TemporaryDirectoryAllocator tmpDir = new TemporaryDirectoryAllocator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use org.junit.TemporaryFolder
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
/** Executes a shell script build on a Swarm Client agent. */ | ||
@Test | ||
public void buildShellScript() throws Exception { | ||
story.addStep( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are in Java 8, it is nicer to use .then
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
project.setDefinition(new CpsFlowDefinition(getFlow(node, 0), true)); | ||
|
||
WorkflowRun build = project.scheduleBuild2(0).waitForStart(); | ||
story.j.waitForCompletion(build); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WorkflowRun build = story.j.buildAndAssertSuccess(project);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
File output = new File(tmpDir, "swarm-client.jar"); | ||
try (InputStream inputStream = input.openStream(); | ||
FileOutputStream outputStream = new FileOutputStream(output)) { | ||
ByteStreams.copy(inputStream, outputStream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there i a FileUtils.copy
or similar that makes that simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
return output; | ||
} | ||
|
||
/** Wait for the agent with the given name to come online against the given Jenkins instance. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a method for this in JenkinsRule
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried using JenkinsRule#waitOnline
, but I wasn't able to make it work. That method takes as input a Slave
, and I don't have a Slave
to pass into it because the Slave
is created by the Swarm plugin and not my test code.
StringBuilder sb = new StringBuilder(); | ||
sb.append("\"" + System.getProperty("java.home") + "/bin/java\""); | ||
sb.append(" -Djava.awt.headless=true"); | ||
sb.append(" -jar \"" + swarmClientJar.getAbsolutePath() + "\""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sb.append(" -jar \"" + swarmClientJar.getAbsolutePath() + "\""); | |
sb.append(" -jar \"" + swarmClientJar + "\""); |
(toString
already gets the path)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
sb.append(" " + arg); | ||
} | ||
|
||
ProcessBuilder pb = new ProcessBuilder(Util.tokenize(sb.toString())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, simpler to construct a List<String> argv
to begin with, and dispense with the quoting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
plugin/pom.xml
Outdated
<dependency> | ||
<groupId>org.jenkins-ci.plugins</groupId> | ||
<artifactId>scm-api</artifactId> | ||
<version>1.3</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things like this you probably want in dependencyManagement
, since they are not direct dependencies, pending JENKINS-47498.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Thanks for the review! |
project.setDefinition(new CpsFlowDefinition(getFlow(node, 1), true)); | ||
|
||
WorkflowRun build = project.scheduleBuild2(0).waitForStart(); | ||
SemaphoreStep.waitForStart("wait-0/1", build); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that since this is waiting in node
but not sh
it only exercises part of jenkinsci/workflow-durable-task-step-plugin#104 (when jenkinsci/workflow-durable-task-step-plugin@58b127f was added).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that since this is waiting in
node
but notsh
it only exercises part of jenkinsci/workflow-durable-task-step-plugin#104
Good point. I added a new test, PipelineJobTest#buildShellScriptAcrossDisconnect
, which waits in sh
(essentially a port of ExecutorStepTest#buildShellScriptAcrossDisconnect
).
6a96558
to
8b4fa2e
Compare
@oleg-nenashev I think I addressed all of your feedback above. Do you have any objection to proceeding with this change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation in #81 (comment) . I think we can go forward with this PR
See JENKINS-36013. The Swarm Plugin doesn't have any integration tests that automatically run the Swarm Client against a real Jenkins master. Such tests would be highly desirable to prevent future regressions.
This change adds two new test classes:
SwarmClientIntegrationTest
(which tests the Swarm Client using a Freestyle job) andPipelineTest
(which tests the Swarm Client using a Pipeline job). The tests work by downloading the Swarm Client from the Jenkins master and using it to connect to Jenkins.