Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/main/java/com/sonyericsson/rebuild/RebuildAction.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
*/
package com.sonyericsson.rebuild;

import com.google.common.collect.*;
import hudson.Extension;
import hudson.model.Action;

Expand All @@ -32,6 +33,8 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.Comparator;

import hudson.matrix.MatrixRun;
import hudson.model.BooleanParameterValue;
Expand All @@ -57,9 +60,6 @@
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;

import com.sonyericsson.rebuild.RebuildParameterPage;
import com.sonyericsson.rebuild.RebuildParameterProvider;

/**
* Rebuild RootAction implementation class. This class will basically reschedule
* the build with existing parameters.
Expand Down Expand Up @@ -297,7 +297,13 @@ public void doConfigSubmit(StaplerRequest req, StaplerResponse rsp) throws Servl
}
}

List<Action> actions = constructRebuildCause(build, new ParametersAction(values));
Set<ParameterValue> mergedValues = ImmutableSortedSet.copyOf(new Comparator<ParameterValue>() {
@Override
public int compare(ParameterValue o1, ParameterValue o2) {
return o1.getName().compareTo(o2.getName());
}
}, Iterables.concat(values, paramAction.getParameters()));
Copy link
Member

Choose a reason for hiding this comment

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

You should check for null before accessing paramAction

Copy link
Author

Choose a reason for hiding this comment

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

@amuniz Added the null check to be safe but struggling to see how 'paramAction' would be null at this stage based on - https://github.com/jenkinsci/rebuild-plugin/blob/master/src/main/java/com/sonyericsson/rebuild/RebuildAction.java#L206

The lack of null checks in the existing code seems to echo this - https://github.com/jenkinsci/rebuild-plugin/blob/master/src/main/java/com/sonyericsson/rebuild/RebuildAction.java#L417

Either that or its just really fragile :)

cheers

Copy link
Member

Choose a reason for hiding this comment

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

Probably you are right, the action will be always set and it's never null, but, in general, I think it's a good practice to check for null the return value of getAction, since it depends on someone adding that action before (and in some cases even in a separate plugin, which is clearly butterfly effect prone 😄 IMO)

List<Action> actions = constructRebuildCause(build, new ParametersAction(Lists.newArrayList(mergedValues)));
Hudson.getInstance().getQueue().schedule((Queue.Task) build.getParent(), 0, actions);

rsp.sendRedirect("../../");
Expand Down Expand Up @@ -361,14 +367,14 @@ public boolean isRebuildAvailable() {
&& project.hasPermission(Item.BUILD)
&& project.isBuildable()
&& project instanceof Queue.Task
&& !isMatrixRun()
&& !isMatrixRun()
&& !isRebuildDisbaled();

Choose a reason for hiding this comment

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

you have some formatting changes


}

private boolean isRebuildDisbaled() {
RebuildSettings settings = (RebuildSettings)getProject().getProperty(RebuildSettings.class);

if (settings != null && settings.getRebuildDisabled()) {
return true;
}
Expand Down
109 changes: 109 additions & 0 deletions src/test/java/com/sonyericsson/rebuild/RebuildValidatorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@
import java.util.List;
import java.util.concurrent.ExecutionException;

import static com.sonyericsson.rebuild.matchers.IsCollectionContainingStringParameterValues.hasStringParamValues;
import static com.sonyericsson.rebuild.matchers.IsNotCollectionContainingStringParameterValues.hasNoStringParamValues;
import static com.sonyericsson.rebuild.matchers.StringParameterValueMatcher.equalToStringParamValue;
import static org.junit.Assert.assertThat;

/**
* For testing the extension point.
*
Expand Down Expand Up @@ -393,6 +398,110 @@ public void testRebuildSupportedUnknownParameterValue() throws Exception {
page.asText().contains("This is a mark for test"));
}

public void testWhenProjectWithExistingParamAndOverridingParamWhenRebuild()
throws Exception {

Choose a reason for hiding this comment

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

I would suggest to rename this test, I think there's something wrong with the sentence itself and it is not really clear what's the goal of the test, probably something like "newParametersShouldOverrideExistingPatametersIfHaveSameName" or something like that.

Copy link
Author

Choose a reason for hiding this comment

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

changed

FreeStyleProject project = createFreeStyleProject();
project.addProperty(new ParametersDefinitionProperty(
new StringParameterDefinition("existing", "defaultValue")));

// Build (#1)
project.scheduleBuild2(0, new Cause.UserIdCause(),
new ParametersAction(new StringParameterValue("existing", "overridingValue")))
.get();
HtmlPage rebuildConfigPage = createWebClient().getPage(project,
"1/rebuild");
// Rebuild (#2)
submit(rebuildConfigPage.getFormByName("config"));

List<ParameterValue> parameterValues = project.getBuildByNumber(2).getAction(ParametersAction.class).getParameters();

assertThat(parameterValues, hasStringParamValues(equalToStringParamValue("existing", "overridingValue")));
assertThat(parameterValues, hasNoStringParamValues(equalToStringParamValue("existing", "defaultValue")));
}

public void testWhenProjectWithExistingParamAndOverridingParamWhenRebuildOfLastBuild()
throws Exception {

Choose a reason for hiding this comment

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

same here, something like "rebuildingLastBuildShouldMaintainExistingParameters"

Copy link
Author

Choose a reason for hiding this comment

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

changed

FreeStyleProject project = createFreeStyleProject();
project.addProperty(new ParametersDefinitionProperty(
new StringParameterDefinition("existing", "defaultValue")));

// Build (#1)
project.scheduleBuild2(0, new Cause.UserIdCause(),
new ParametersAction(new StringParameterValue("existing", "overridingValue")))
.get();
HtmlPage rebuildConfigPage = createWebClient().getPage(project,
"1/rebuild");
// Rebuild (#2)
submit(rebuildConfigPage.getFormByName("config"));

HtmlPage projectPage = createWebClient().getPage(project);
WebAssert.assertLinkPresentWithText(projectPage, "Rebuild Last");

HtmlAnchor rebuildHref = projectPage.getAnchorByText("Rebuild Last");
assertEquals("Rebuild Last should point to the second build", "/"
+ project.getUrl() + "lastCompletedBuild/rebuild",
rebuildHref.getHrefAttribute());

List<ParameterValue> parameterValues = project.getLastCompletedBuild().getAction(ParametersAction.class).getParameters();

assertThat(parameterValues, hasStringParamValues(equalToStringParamValue("existing", "overridingValue")));
assertThat(parameterValues, hasNoStringParamValues(equalToStringParamValue("existing", "defaultValue")));
}

public void testWhenProjectWithExistingParamAndInjectedParamWhenRebuild()

Choose a reason for hiding this comment

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

same

Copy link
Author

Choose a reason for hiding this comment

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

changed

throws Exception {
FreeStyleProject project = createFreeStyleProject();
project.addProperty(new ParametersDefinitionProperty(
new StringParameterDefinition("oldName", "oldValue")));

// Build (#1)
project.scheduleBuild2(0, new Cause.UserIdCause(),
new ParametersAction(new StringParameterValue("injectedName", "injectedValue")))
.get();
HtmlPage rebuildConfigPage = createWebClient().getPage(project,
"1/rebuild");
// Rebuild (#2)
submit(rebuildConfigPage.getFormByName("config"));

List<ParameterValue> parameterValues = project.getBuildByNumber(2).getAction(ParametersAction.class).getParameters();

assertThat(parameterValues, hasStringParamValues(
equalToStringParamValue("oldName", "oldValue"),
equalToStringParamValue("injectedName", "injectedValue")
));
}

public void testWhenProjectWithExistingParamAndInjectedParamWhenRebuildOfLastBuild()
throws Exception {

Choose a reason for hiding this comment

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

same

Copy link
Author

Choose a reason for hiding this comment

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

changed

FreeStyleProject project = createFreeStyleProject();
project.addProperty(new ParametersDefinitionProperty(
new StringParameterDefinition("oldName", "oldValue")));

// Build (#1)
project.scheduleBuild2(0, new Cause.UserIdCause(),
new ParametersAction(new StringParameterValue("injectedName", "injectedValue")))
.get();
HtmlPage rebuildConfigPage = createWebClient().getPage(project,
"1/rebuild");
// Rebuild (#2)
submit(rebuildConfigPage.getFormByName("config"));

HtmlPage projectPage = createWebClient().getPage(project);
WebAssert.assertLinkPresentWithText(projectPage, "Rebuild Last");

HtmlAnchor rebuildHref = projectPage.getAnchorByText("Rebuild Last");
assertEquals("Rebuild Last should point to the second build", "/"
+ project.getUrl() + "lastCompletedBuild/rebuild",
rebuildHref.getHrefAttribute());

List<ParameterValue> parameterValues = project.getLastCompletedBuild().getAction(ParametersAction.class).getParameters();

assertThat(parameterValues, hasStringParamValues(
equalToStringParamValue("oldName", "oldValue"),
equalToStringParamValue("injectedName", "injectedValue")
));
}

/**
* A parameter value rebuild plugin does not know.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package com.sonyericsson.rebuild.matchers;

import hudson.model.ParameterValue;
import org.hamcrest.Description;
import org.junit.internal.matchers.TypeSafeMatcher;

import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public final class IsCollectionContainingStringParameterValues extends TypeSafeMatcher<Collection<ParameterValue>> {

Choose a reason for hiding this comment

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

I will refactor this to be the StringParameterValuesMatcher and I would delete the IsNotCollectionContainingStringParameterValues and the current StringParameterValuesMatcher (from this class you just need the matches method since the constructor just takes the StringParameterValue constructor).

Copy link
Author

Choose a reason for hiding this comment

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

agreed & changed

private final List<StringParameterValueMatcher> matchers;

public static IsCollectionContainingStringParameterValues hasStringParamValues(StringParameterValueMatcher... items) {
return new IsCollectionContainingStringParameterValues(items);
}

IsCollectionContainingStringParameterValues(StringParameterValueMatcher... items) {
this.matchers = new ArrayList<StringParameterValueMatcher>(items.length);
for (StringParameterValueMatcher matcher : items) {
matchers.add(matcher);
}
}

@Override
public boolean matchesSafely(Collection<ParameterValue> items) {

Choose a reason for hiding this comment

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

are you ever calling this method?unless I am missing something, should not this be called by the hasStringParamValues method?

Copy link
Author

Choose a reason for hiding this comment

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

called by hamcrest

Map<StringParameterValueMatcher, Boolean> resultMap = new HashMap<StringParameterValueMatcher, Boolean>();
for (StringParameterValueMatcher stringParameterValueMatcher : matchers) {
resultMap.put(stringParameterValueMatcher, false);
for (ParameterValue item : items) {
if (stringParameterValueMatcher.matches(item)) {
resultMap.replace(stringParameterValueMatcher, true);
break;
}
}
}
return !resultMap.values().contains(false);
}

@Override
public void describeTo(Description description) {
description.appendList("<[", ", ", "]>", matchers);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package com.sonyericsson.rebuild.matchers;

import hudson.model.ParameterValue;
import org.hamcrest.Description;
import org.junit.internal.matchers.TypeSafeMatcher;

import java.util.Collection;

public final class IsNotCollectionContainingStringParameterValues extends TypeSafeMatcher<Collection<ParameterValue>> {

Choose a reason for hiding this comment

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

I don't think you really need this class, you could just negate the result of hasStringParamValues

Copy link
Author

Choose a reason for hiding this comment

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

agreed & changed


private final IsCollectionContainingStringParameterValues delegate;

public static IsNotCollectionContainingStringParameterValues hasNoStringParamValues(StringParameterValueMatcher... items) {
return new IsNotCollectionContainingStringParameterValues(items);
}

private IsNotCollectionContainingStringParameterValues(StringParameterValueMatcher... items) {
delegate = new IsCollectionContainingStringParameterValues(items);
}

@Override
public boolean matchesSafely(Collection<ParameterValue> items) {
return !delegate.matchesSafely(items);
}

@Override
public void describeTo(Description description) {
delegate.describeTo(description);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package com.sonyericsson.rebuild.matchers;

import hudson.model.StringParameterValue;
import org.hamcrest.BaseMatcher;
import org.hamcrest.Description;

public final class StringParameterValueMatcher extends BaseMatcher<StringParameterValue> {

private final StringParameterValue thisParameterValue;

StringParameterValueMatcher(StringParameterValue thisParameterValue) {
this.thisParameterValue = thisParameterValue;
}

public static StringParameterValueMatcher equalToStringParamValue(String name, String value) {
return new StringParameterValueMatcher(new StringParameterValue(name, value));

Choose a reason for hiding this comment

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

is this method really needed? you could just call the constructor

Copy link
Author

Choose a reason for hiding this comment

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

agreed & changed

}

@Override
public boolean matches(Object parameterValue) {

Choose a reason for hiding this comment

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

I would say the input should be called parameter not parameterValue which can be confused with what in fact is parameter.value

Copy link
Author

Choose a reason for hiding this comment

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

agreed & changed

return parameterValue instanceof StringParameterValue
&& thisParameterValue.getName().equals(((StringParameterValue)parameterValue).getName())
&& thisParameterValue.value.equals(((StringParameterValue)parameterValue).value);
}

@Override
public void describeTo(Description description) {
description.appendText(thisParameterValue.toString());
}
}