From 2b0f02c12b67e949dc3fa1ad865bc518fc89e35c Mon Sep 17 00:00:00 2001 From: Marco Miller Date: Fri, 13 Mar 2015 12:37:04 -0400 Subject: [PATCH 1/3] Add tests for copyAllScoresIfNoChange in LabelTypeIT As no tests were added for copyAllScoresIfNoChange since the latter was introduced (by commit ae47686). So now there are tests covering that feature, including logic such as the one changed in the parent of this very (hereby) commit. Another commit is being drafted to address the need for LabelTypeIT setUp to stop overriding initial booleans for copy-score flags. Goal being to have setUp assume ProjectConfig / LabelType defaults instead. LabelTypeIT tests are thus changed by that other commit, accordingly. Change-Id: I1502cc40bf4b387418c887a7626386dd11fc2a87 --- .../server/project/LabelTypeIT.java | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java index e9e3ffbc36..eb50d3d221 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java @@ -52,6 +52,7 @@ public class LabelTypeIT extends AbstractDaemonTest { codeReview.setCopyMaxScore(false); codeReview.setCopyAllScoresOnTrivialRebase(false); codeReview.setCopyAllScoresIfNoCodeChange(false); + codeReview.setCopyAllScoresIfNoChange(false); codeReview.setDefaultValue((short)-1); saveProjectConfig(cfg); } @@ -122,6 +123,22 @@ public class LabelTypeIT extends AbstractDaemonTest { assertApproval(r, 0); } + @Test + public void noCopyAllScoresIfNoChange() throws Exception { + PushOneCommit.Result patchSet = readyPatchSetForNoChangeRebase(); + rebase(patchSet); + assertApproval(patchSet, 0); + } + + @Test + public void copyAllScoresIfNoChange() throws Exception { + PushOneCommit.Result patchSet = readyPatchSetForNoChangeRebase(); + codeReview.setCopyAllScoresIfNoChange(true); + saveLabelConfig(); + rebase(patchSet); + assertApproval(patchSet, 1); + } + @Test public void noCopyAllScoresIfNoCodeChange() throws Exception { String file = "a.txt"; @@ -267,6 +284,34 @@ public class LabelTypeIT extends AbstractDaemonTest { .get()); } + private PushOneCommit.Result readyPatchSetForNoChangeRebase() + throws Exception { + String file = "a.txt"; + String contents = "contents"; + + PushOneCommit push = pushFactory.create(db, admin.getIdent(), + PushOneCommit.SUBJECT, file, contents); + PushOneCommit.Result base = push.to(git, "refs/for/master"); + merge(base); + + push = pushFactory.create(db, admin.getIdent(), + PushOneCommit.SUBJECT, file, contents + "M"); + PushOneCommit.Result basePlusM = push.to(git, "refs/for/master"); + merge(basePlusM); + + push = pushFactory.create(db, admin.getIdent(), + PushOneCommit.SUBJECT, file, contents); + PushOneCommit.Result basePlusMMinusM = push.to(git, "refs/for/master"); + merge(basePlusMMinusM); + + git.checkout().setName(base.getCommit().name()).call(); + push = pushFactory.create(db, admin.getIdent(), + PushOneCommit.SUBJECT, file, contents + "MM"); + PushOneCommit.Result patchSet = push.to(git, "refs/for/master"); + revision(patchSet).review(ReviewInput.recommend()); + return patchSet; + } + private void saveLabelConfig() throws Exception { ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig(); cfg.getLabelSections().clear(); From 5a65d032f39211145b146d5da0e45a485ee65324 Mon Sep 17 00:00:00 2001 From: Marco Miller Date: Fri, 13 Mar 2015 13:14:17 -0400 Subject: [PATCH 2/3] Remove duplication of defaults for boolean label types in ProjectConfig Move all defaults for copy-score (and canOverride, similar) booleans from multiple locations in ProjectConfig to LabelType. The latter then has the sole occurrence of each involved default value (constant-s). ProjectConfig is refactored accordingly, including a method that sets those boolean config keys through reusing the aforementioned constants. Change-Id: Icfdd3ef24aec922cda2a41ff9e019225c4f7243e --- .../google/gerrit/common/data/LabelType.java | 7 ++ .../gerrit/server/git/ProjectConfig.java | 72 +++++++++---------- 2 files changed, 43 insertions(+), 36 deletions(-) diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java index 315be845bd..2abe68559d 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java @@ -25,6 +25,13 @@ import java.util.List; import java.util.Map; public class LabelType { + public static final boolean DEF_CAN_OVERRIDE = true; + public static final boolean DEF_COPY_ALL_SCORES_IF_NO_CHANGE = true; + public static final boolean DEF_COPY_ALL_SCORES_IF_NO_CODE_CHANGE = false; + public static final boolean DEF_COPY_ALL_SCORES_ON_TRIVIAL_REBASE = false; + public static final boolean DEF_COPY_MAX_SCORE = false; + public static final boolean DEF_COPY_MIN_SCORE = false; + public static LabelType withDefaultValues(String name) { checkName(name); List values = new ArrayList<>(2); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java index c749a2d9db..020c94ae71 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java @@ -691,17 +691,23 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. KEY_DEFAULT_VALUE, dv, name))); } label.setCopyMinScore( - rc.getBoolean(LABEL, name, KEY_COPY_MIN_SCORE, false)); + rc.getBoolean(LABEL, name, KEY_COPY_MIN_SCORE, + LabelType.DEF_COPY_MIN_SCORE)); label.setCopyMaxScore( - rc.getBoolean(LABEL, name, KEY_COPY_MAX_SCORE, false)); + rc.getBoolean(LABEL, name, KEY_COPY_MAX_SCORE, + LabelType.DEF_COPY_MAX_SCORE)); label.setCopyAllScoresOnTrivialRebase( - rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, false)); + rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, + LabelType.DEF_COPY_ALL_SCORES_ON_TRIVIAL_REBASE)); label.setCopyAllScoresIfNoCodeChange( - rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE, false)); + rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE, + LabelType.DEF_COPY_ALL_SCORES_IF_NO_CODE_CHANGE)); label.setCopyAllScoresIfNoChange( - rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, true)); + rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, + LabelType.DEF_COPY_ALL_SCORES_IF_NO_CHANGE)); label.setCanOverride( - rc.getBoolean(LABEL, name, KEY_CAN_OVERRIDE, true)); + rc.getBoolean(LABEL, name, KEY_CAN_OVERRIDE, + LabelType.DEF_CAN_OVERRIDE)); label.setRefPatterns(getStringListOrNull(rc, LABEL, name, KEY_Branch)); labelSections.put(name, label); } @@ -1034,37 +1040,22 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. toUnset.remove(name); rc.setString(LABEL, name, KEY_FUNCTION, label.getFunctionName()); rc.setInt(LABEL, name, KEY_DEFAULT_VALUE, label.getDefaultValue()); - if (label.isCopyMinScore()) { - rc.setBoolean(LABEL, name, KEY_COPY_MIN_SCORE, true); - } else { - rc.unset(LABEL, name, KEY_COPY_MIN_SCORE); - } - if (label.isCopyMaxScore()) { - rc.setBoolean(LABEL, name, KEY_COPY_MAX_SCORE, true); - } else { - rc.unset(LABEL, name, KEY_COPY_MAX_SCORE); - } - if (label.isCopyAllScoresOnTrivialRebase()) { - rc.setBoolean(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, true); - } else { - rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE); - } - if (label.isCopyAllScoresIfNoCodeChange()) { - rc.setBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE, true); - } else { - rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE); - } - if (!label.isCopyAllScoresIfNoChange()) { - rc.setBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, false); - } else { - rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE); - } - if (!label.canOverride()) { - rc.setBoolean(LABEL, name, KEY_CAN_OVERRIDE, false); - } else { - rc.unset(LABEL, name, KEY_CAN_OVERRIDE); - } + setBooleanConfigKey(rc, name, KEY_COPY_MIN_SCORE, label.isCopyMinScore(), + LabelType.DEF_COPY_MIN_SCORE); + setBooleanConfigKey(rc, name, KEY_COPY_MAX_SCORE, label.isCopyMaxScore(), + LabelType.DEF_COPY_MAX_SCORE); + setBooleanConfigKey(rc, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, + label.isCopyAllScoresOnTrivialRebase(), + LabelType.DEF_COPY_ALL_SCORES_ON_TRIVIAL_REBASE); + setBooleanConfigKey(rc, name, KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE, + label.isCopyAllScoresIfNoCodeChange(), + LabelType.DEF_COPY_ALL_SCORES_IF_NO_CODE_CHANGE); + setBooleanConfigKey(rc, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, + label.isCopyAllScoresIfNoChange(), + LabelType.DEF_COPY_ALL_SCORES_IF_NO_CHANGE); + setBooleanConfigKey(rc, name, KEY_CAN_OVERRIDE, label.canOverride(), + LabelType.DEF_CAN_OVERRIDE); List values = Lists.newArrayListWithCapacity(label.getValues().size()); for (LabelValue value : label.getValues()) { @@ -1078,6 +1069,15 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. } } + private static void setBooleanConfigKey( + Config rc, String name, String key, boolean value, boolean defaultValue) { + if (value == defaultValue) { + rc.unset(LABEL, name, key); + } else { + rc.setBoolean(LABEL, name, key, value); + } + } + private void savePluginSections(Config rc) { List existing = Lists.newArrayList(rc.getSubsections(PLUGIN)); for (String name : existing) { From 5f834717cc30d2c66aa2a048b95a169c66d2a2e1 Mon Sep 17 00:00:00 2001 From: Marco Miller Date: Fri, 13 Mar 2015 17:20:44 -0400 Subject: [PATCH 3/3] Make LabelTypeIT tests assume copy-score defaults now initialized herein Remove overriding settings from LabelTypeIT setUp so that those tests assume defaults defined in LabelType (and exercised in ProjectConfig). Add initialization of such defaults to LabelType constructor; otherwise, defaults set to true were not initialized as such, served false instead. Add mentions of false defaults to Documentation, for copyMinScore and copyMaxScore (used to be missing). Only allProjects had its copyMinScore default set to true, in AllProjectsCreator (initCodeReviewLabel). Change-Id: I7957ff8d628d7e1744a910e66dcae6b41343a7e7 --- Documentation/config-labels.txt | 5 +++-- .../acceptance/server/project/LabelTypeIT.java | 15 +++++++-------- .../com/google/gerrit/common/data/LabelType.java | 6 ++++++ 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt index b20cd74131..3c0f0ebdc4 100644 --- a/Documentation/config-labels.txt +++ b/Documentation/config-labels.txt @@ -222,7 +222,8 @@ determining whether a change is submittable. === `label.Label-Name.copyMinScore` If true, the lowest possible negative value for the label is copied -forward when a new patch set is uploaded. +forward when a new patch set is uploaded. Defaults to false, except +for All-Projects which has it true by default. [[label_copyMaxScore]] === `label.Label-Name.copyMaxScore` @@ -230,7 +231,7 @@ forward when a new patch set is uploaded. If true, the highest possible positive value for the label is copied forward when a new patch set is uploaded. This can be used to enable sticky approvals, reducing turn-around for trivial cleanups prior to -submitting a change. +submitting a change. Defaults to false. [[label_copyAllScoresOnTrivialRebase]] === `label.Label-Name.copyAllScoresOnTrivialRebase` diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java index eb50d3d221..efb461511b 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java @@ -48,17 +48,16 @@ public class LabelTypeIT extends AbstractDaemonTest { public void setUp() throws Exception { ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig(); codeReview = checkNotNull(cfg.getLabelSections().get("Code-Review")); - codeReview.setCopyMinScore(false); - codeReview.setCopyMaxScore(false); - codeReview.setCopyAllScoresOnTrivialRebase(false); - codeReview.setCopyAllScoresIfNoCodeChange(false); - codeReview.setCopyAllScoresIfNoChange(false); codeReview.setDefaultValue((short)-1); saveProjectConfig(cfg); } @Test public void noCopyMinScoreOnRework() throws Exception { + //allProjects only has it true by default + codeReview.setCopyMinScore(false); + saveLabelConfig(); + PushOneCommit.Result r = createChange(); revision(r).review(ReviewInput.reject()); assertApproval(r, -2); @@ -72,7 +71,7 @@ public class LabelTypeIT extends AbstractDaemonTest { saveLabelConfig(); PushOneCommit.Result r = createChange(); revision(r).review(ReviewInput.reject()); - //assertApproval(r, -2); + assertApproval(r, -2); r = amendChange(r.getChangeId()); assertApproval(r, -2); } @@ -125,6 +124,8 @@ public class LabelTypeIT extends AbstractDaemonTest { @Test public void noCopyAllScoresIfNoChange() throws Exception { + codeReview.setCopyAllScoresIfNoChange(false); + saveLabelConfig(); PushOneCommit.Result patchSet = readyPatchSetForNoChangeRebase(); rebase(patchSet); assertApproval(patchSet, 0); @@ -133,8 +134,6 @@ public class LabelTypeIT extends AbstractDaemonTest { @Test public void copyAllScoresIfNoChange() throws Exception { PushOneCommit.Result patchSet = readyPatchSetForNoChangeRebase(); - codeReview.setCopyAllScoresIfNoChange(true); - saveLabelConfig(); rebase(patchSet); assertApproval(patchSet, 1); } diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java index 2abe68559d..cf6f7564fb 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java @@ -134,6 +134,12 @@ public class LabelType { maxPositive = values.get(values.size() - 1).getValue(); } } + setCanOverride(DEF_CAN_OVERRIDE); + setCopyAllScoresIfNoChange(DEF_COPY_ALL_SCORES_IF_NO_CHANGE); + setCopyAllScoresIfNoCodeChange(DEF_COPY_ALL_SCORES_IF_NO_CODE_CHANGE); + setCopyAllScoresOnTrivialRebase(DEF_COPY_ALL_SCORES_ON_TRIVIAL_REBASE); + setCopyMaxScore(DEF_COPY_MAX_SCORE); + setCopyMinScore(DEF_COPY_MIN_SCORE); } public String getName() {