Add build step with timeout feature #61

Jimilian · 2016-10-24T15:30:43Z

Sometimes it's much better to have several small timeouts instead of one global one.

ikedam

Do you plan to add test codes?

ikedam · 2016-10-25T11:17:16Z

src/main/java/hudson/plugins/build_timeout/BuildStepWithTimeout.java

+
+import hudson.Extension;
+import hudson.Launcher;
+import hudson.model.*;


Please don't use wildcard imports.

ikedam · 2016-10-25T11:18:44Z

src/main/java/hudson/plugins/build_timeout/BuildStepWithTimeout.java

+
+
+public class BuildStepWithTimeout extends Builder implements BuildStep {
+    private /* final */ BuildTimeOutStrategy strategy;


This can be final.

ikedam · 2016-10-25T11:20:02Z

src/main/java/hudson/plugins/build_timeout/BuildStepWithTimeout.java

+
+public class BuildStepWithTimeout extends Builder implements BuildStep {
+    private /* final */ BuildTimeOutStrategy strategy;
+    private BuildStep buildStep;


Would you consider to accept multiple steps?
That should make this feature more useful.

You can do that by using <f:hetero-list> instead of <f:dropdownDescriptor>.
This review may be helpful: https://github.com/jenkinsci/flexible-publish-plugin/pull/6/files

To be honest, I think it's too dangerous :)

From my perspective somebody should implement "multiple steps" build step once and all other plugins should just re-use it. I don't think that it's good idea to re-implement such logic, because even now I can't guarantee that all kind of build steps are supported (i.e. conditional steps doesn't work if it's used at child).

And in my case I don't need multiple steps - this "feature" was designed to split one global timeout on smallest chunks.

Actually, wrapping another buildstep is always dangerous.
Problems should happen even wrapping a single buildstep.
I can't get why you think it gets more dangerous when supporting multiple steps.

conditional steps doesn't work if it's used at child

I'll see why this happen.

Anyway, supporting multiple steps isn't required for this request.

ikedam · 2016-10-25T11:24:50Z

src/main/java/hudson/plugins/build_timeout/BuildStepWithTimeout.java

+
+    public void setBuildStep(BuildStep buildStep) {
+        this.buildStep = buildStep;
+    }


I don't think you need this setter, and buildStep can be final.

ikedam · 2016-10-25T11:28:15Z

src/main/java/hudson/plugins/build_timeout/BuildStepWithTimeout.java

+
+
+    public boolean perform(final Build<?,?> build, final Launcher launcher, final BuildListener listener) throws InterruptedException, IOException {
+        final Timer timer = new Timer("Timer-" + build.getDisplayName().replace(" ", "_"), true);


You can use Trigger.timer as in BuildTimeoutWrapper.
http://javadoc.jenkins-ci.org/hudson/triggers/Trigger.html#timer

ikedam · 2016-10-25T11:40:08Z

src/main/java/hudson/plugins/build_timeout/BuildStepWithTimeout.java

+            return "Run with timeout";
+        }
+
+        public boolean isApplicable(Class<? extends AbstractProject> jobType) {


Let's annotate with Override.

ikedam · 2016-10-25T11:43:22Z

src/main/java/hudson/plugins/build_timeout/BuildStepWithTimeout.java

+            BuildStep buildstep = BuildTimeOutUtility.bindJSONWithDescriptor(req, formData, "buildStep", BuildStep.class);
+            BuildTimeOutStrategy strategy = BuildTimeOutUtility.bindJSONWithDescriptor(req, formData, "strategy", BuildTimeOutStrategy.class);
+
+            if (!(strategy instanceof AbsoluteTimeOutStrategy || strategy instanceof DeadlineTimeOutStrategy)) {


Add a new method like TimeOutStrategy#isApplicableAsBuildStep and filter with that.
That's not a good idea to list up applicable classes in codes.

ikedam · 2016-10-25T11:44:17Z

src/main/java/hudson/plugins/build_timeout/BuildStepWithTimeout.java

+        }
+
+        public List<BuildTimeOutStrategyDescriptor> getStrategies() {
+            return Jenkins.getInstance().getDescriptorList(BuildTimeOutStrategy.class);


You can apply TimeOutStrategy#isApplicableAsBuildStep also here.

ikedam · 2016-10-25T11:46:07Z

src/main/java/hudson/plugins/build_timeout/operations/AbortOperation.java

@@ -65,6 +65,8 @@ public boolean perform(AbstractBuild<?, ?> build, BuildListener listener, long e
        if (e != null) {
            e.interrupt(Result.ABORTED);
        }
+        build.setResult(Result.ABORTED);


What happens if you don't add this?
(You mean e.interrupt doesn't work?)

Yes, it doesn't work for some reason. Interruption itself works as expected, but build result is different.

The result set with e.inturrupt isn't used as you suppressed InterruptedException.
I don't think it's good idea to suppress InterruptException as it may cause Jenkins work differently when you use BuildTimeOutWrapper and BuildStepWithTimeout.

As far as I tested the behavior, stacktraces aren't output into build logs (console logs), but are output only into system logs.
Can I know more details about why you want to suppress InterruptedException?

ikedam · 2016-10-25T11:47:53Z

src/main/java/hudson/plugins/build_timeout/operations/FailOperation.java

@@ -65,6 +61,7 @@ public boolean perform(AbstractBuild<?, ?> build, BuildListener listener, long e
        if (e != null) {
            e.interrupt(Result.FAILURE);
        }
+        build.setResult(Result.FAILURE);


Jimilian · 2016-10-26T15:22:50Z

Ok, everything is done (I hope). I added a couple of tests for basic mechanic.

ikedam · 2016-10-30T05:24:17Z

src/test/java/hudson/plugins/build_timeout/BuildStepWithTimeoutTest.java

+    }
+
+    @Test
+    public void testTimeoutWasNotTriggered() throws IOException, ExecutionException, InterruptedException {


JFYI: It's easy for you to use throws Exception in test codes.

ikedam · 2016-10-30T05:25:14Z

src/test/java/hudson/plugins/build_timeout/BuildStepWithTimeoutTest.java

+        project.getBuildersList().add(step);
+
+        Future<FreeStyleBuild> future = project.scheduleBuild2(0, new Cause.UserIdCause());
+        FreeStyleBuild build = future.get();


You have to test the result of the build with JenkinsRule#assertBuildStatusSuccess or JenkinsRule#assertBuildStatus

ikedam · 2016-10-30T05:26:15Z

src/test/java/hudson/plugins/build_timeout/BuildStepWithTimeoutTest.java

+        Future<FreeStyleBuild> future = project.scheduleBuild2(0, new Cause.UserIdCause());
+        FreeStyleBuild build = future.get();
+
+        assertTrue(IOUtils.toString(build.getLogReader()).contains("Test"));


JFYI: JenkinsRule#assertLogContains is useful (but there isn't assertLogNotContains...)

ikedam · 2016-10-30T05:28:24Z

src/main/java/hudson/plugins/build_timeout/BuildStepWithTimeout.java

+
+        try {
+            timer.schedule(task, delay);
+            return buildStep.perform((AbstractBuild) build, launcher, listener);


Please add generic types (AbstractBuild<?, ?>) as they are warned by compilers.

ikedam · 2016-10-30T05:36:05Z

Would you tell me more details about "conditional steps doesn't work if it's used at child" ?
As far as I tested, it looks work with BuildStepWithTimeout as expected
(tested with Jenkins 1.651 and condition Always and Never work as expected).

I believe this is an important issue as conditional buildstep can be used as "multiple steps".

Also: Add new tests to verify build results

Jimilian · 2016-10-31T09:25:32Z

Thanks for your suggestions - I made some refactoring for tests. Now even I'm satisfied with them :)

Would you tell me more details about "conditional steps doesn't work if it's used at child" ?

I don't have many details... just empty list of builders. No logs, no asserts, nothing in js console as well.

This happens on Jenkins ver. 1.632, but I wasn't able to repeat it on Jenkins ver. 1.651.3.

As far as I tested the behavior, stacktraces aren't output into build logs (console logs), but are output only into system logs.
Can I know more details about why you want to suppress InterruptedException?

I removed suppressing (as well as additional executions of setResult). In first place I mainly tested this plugin with "embedded" Jenkins version (1.466) and in 1.466 user receives exception in console, but this issue doesn't happen in 1.651.3.

ikedam · 2016-11-05T08:15:07Z

As far as I know, InterruptedException isn't printed even in Jenkins-1.466.
Any way, I like the new change. Thanks!

This happens on Jenkins ver. 1.632, but I wasn't able to repeat it on Jenkins ver. 1.651.3.

Sorry, I missed that problem.
It's a bug of f:dropdownDescriptorSelector and fixed in Jenkins 1.645+ (jenkinsci/jenkins#1184).
Plugins can backport f:dropdownDescriptorSelector from Jenkins 1.645+ to fix that problem (like authorize-project-plugin).
You can create a new pull request to do that if you want to fix that problem

ikedam · 2016-11-05T08:16:39Z

I plan to make a new release in the next weekend (11th or 12th Nov)

ikedam · 2016-11-13T04:01:39Z

Released 1.18.
It will be available on the update center in a day.

Add build with step feature

7e7a4d8

Jimilian changed the title ~~Add build with step feature~~ Add build step with timeout feature Oct 24, 2016

ikedam requested changes Oct 25, 2016

View reviewed changes

Fix issues from review

cc8d661

Jimilian force-pushed the build_step_with_timeout branch from d347ac7 to cc8d661 Compare October 26, 2016 15:20

ikedam reviewed Oct 30, 2016

View reviewed changes

Remove duplicated setResult step

78cadde

Also: Add new tests to verify build results

ikedam approved these changes Nov 5, 2016

View reviewed changes

ikedam merged commit 395ea8e into jenkinsci:master Nov 5, 2016

Jimilian deleted the build_step_with_timeout branch November 5, 2016 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add build step with timeout feature #61

Add build step with timeout feature #61

Jimilian commented Oct 24, 2016 •

edited

Loading

ikedam left a comment

ikedam Oct 25, 2016

Jimilian Oct 26, 2016

ikedam Oct 25, 2016

Jimilian Oct 26, 2016

ikedam Oct 25, 2016

Jimilian Oct 26, 2016

ikedam Oct 26, 2016

ikedam Oct 26, 2016

ikedam Oct 25, 2016

Jimilian Oct 26, 2016

ikedam Oct 25, 2016

Jimilian Oct 26, 2016

ikedam Oct 25, 2016

ikedam Oct 25, 2016

ikedam Oct 25, 2016

ikedam Oct 25, 2016

Jimilian Oct 25, 2016

ikedam Oct 30, 2016

ikedam Oct 25, 2016

Jimilian commented Oct 26, 2016

ikedam Oct 30, 2016

ikedam Oct 30, 2016

ikedam Oct 30, 2016

ikedam Oct 30, 2016

ikedam commented Oct 30, 2016

Jimilian commented Oct 31, 2016 •

edited

Loading

ikedam commented Nov 5, 2016

ikedam commented Nov 5, 2016

ikedam commented Nov 13, 2016



		public class BuildStepWithTimeout extends Builder implements BuildStep {
		private /* final */ BuildTimeOutStrategy strategy;



		public boolean perform(final Build<?,?> build, final Launcher launcher, final BuildListener listener) throws InterruptedException, IOException {
		final Timer timer = new Timer("Timer-" + build.getDisplayName().replace(" ", "_"), true);

Add build step with timeout feature #61

Add build step with timeout feature #61

Conversation

Jimilian commented Oct 24, 2016 • edited Loading

ikedam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimilian commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikedam commented Oct 30, 2016

Jimilian commented Oct 31, 2016 • edited Loading

ikedam commented Nov 5, 2016

ikedam commented Nov 5, 2016

ikedam commented Nov 13, 2016

Jimilian commented Oct 24, 2016 •

edited

Loading

Jimilian commented Oct 31, 2016 •

edited

Loading