From ff0569a68a2b57f5b5c5cc7c7ad786c1babc1daf Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 23 Jun 2016 15:28:55 +0900 Subject: [PATCH 1/8] Update issue tracker URL in POM files Change-Id: Ib2130947709532ae3ded532e35f61d8a16c4e332 --- gerrit-acceptance-framework/pom.xml | 4 ++-- gerrit-extension-api/pom.xml | 4 ++-- gerrit-plugin-api/pom.xml | 4 ++-- gerrit-plugin-archetype/pom.xml | 4 ++-- gerrit-plugin-gwt-archetype/pom.xml | 4 ++-- gerrit-plugin-gwtui/pom.xml | 4 ++-- gerrit-plugin-js-archetype/pom.xml | 4 ++-- gerrit-war/pom.xml | 4 ++-- 8 files changed, 16 insertions(+), 16 deletions(-) diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index fedb649681..2153665bb1 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -53,7 +53,7 @@ - http://code.google.com/p/gerrit/issues/list - Google Code Issue Tracker + https://bugs.chromium.org/p/gerrit/issues/list + Gerrit Issue Tracker diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index 532acc9d6d..ab852c40ed 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -53,7 +53,7 @@ - http://code.google.com/p/gerrit/issues/list - Google Code Issue Tracker + https://bugs.chromium.org/p/gerrit/issues/list + Gerrit Issue Tracker diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index 764a7d40d1..70df59a2b0 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -53,7 +53,7 @@ - http://code.google.com/p/gerrit/issues/list - Google Code Issue Tracker + https://bugs.chromium.org/p/gerrit/issues/list + Gerrit Issue Tracker diff --git a/gerrit-plugin-archetype/pom.xml b/gerrit-plugin-archetype/pom.xml index 4208fdf17b..e9841bdd38 100644 --- a/gerrit-plugin-archetype/pom.xml +++ b/gerrit-plugin-archetype/pom.xml @@ -93,7 +93,7 @@ limitations under the License. - http://code.google.com/p/gerrit/issues/list - Google Code Issue Tracker + https://bugs.chromium.org/p/gerrit/issues/list + Gerrit Issue Tracker diff --git a/gerrit-plugin-gwt-archetype/pom.xml b/gerrit-plugin-gwt-archetype/pom.xml index e29f7b89a0..49f679fe65 100644 --- a/gerrit-plugin-gwt-archetype/pom.xml +++ b/gerrit-plugin-gwt-archetype/pom.xml @@ -93,7 +93,7 @@ limitations under the License. - http://code.google.com/p/gerrit/issues/list - Google Code Issue Tracker + https://bugs.chromium.org/p/gerrit/issues/list + Gerrit Issue Tracker diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index c8f842cb0e..1bafcd0662 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -53,7 +53,7 @@ - http://code.google.com/p/gerrit/issues/list - Google Code Issue Tracker + https://bugs.chromium.org/p/gerrit/issues/list + Gerrit Issue Tracker diff --git a/gerrit-plugin-js-archetype/pom.xml b/gerrit-plugin-js-archetype/pom.xml index dcd28416f8..14781adfb7 100644 --- a/gerrit-plugin-js-archetype/pom.xml +++ b/gerrit-plugin-js-archetype/pom.xml @@ -93,7 +93,7 @@ limitations under the License. - http://code.google.com/p/gerrit/issues/list - Google Code Issue Tracker + https://bugs.chromium.org/p/gerrit/issues/list + Gerrit Issue Tracker diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index 8b5e2e364b..260abc5e68 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -53,7 +53,7 @@ - http://code.google.com/p/gerrit/issues/list - Google Code Issue Tracker + https://bugs.chromium.org/p/gerrit/issues/list + Gerrit Issue Tracker From ff8982d5480f0331bb49e87a4454f11607490b25 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 23 Jun 2016 15:30:50 +0900 Subject: [PATCH 2/8] Update issue tracker URL in documentation Change-Id: Ica83202a23678ea3fb93dfcff1f40fbc83c42d66 --- Documentation/dev-design.txt | 2 +- Documentation/error-has-duplicates.txt | 2 +- Documentation/index.txt | 2 +- README.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/dev-design.txt b/Documentation/dev-design.txt index 281e68c2c2..4e82507901 100644 --- a/Documentation/dev-design.txt +++ b/Documentation/dev-design.txt @@ -186,7 +186,7 @@ Gerrit is developed as a self-hosting open source project: * link:https://www.gerritcodereview.com/[Project Homepage] * link:https://www.gerritcodereview.com/download/index.html[Release Versions] * link:https://gerrit.googlesource.com/gerrit[Source] -* link:http://code.google.com/p/gerrit/issues/list[Issue Tracking] +* link:https://bugs.chromium.org/p/gerrit/issues/list[Issue Tracking] * link:https://review.source.android.com/[Change Review] diff --git a/Documentation/error-has-duplicates.txt b/Documentation/error-has-duplicates.txt index 0ec6eda8e8..8294c12ddf 100644 --- a/Documentation/error-has-duplicates.txt +++ b/Documentation/error-has-duplicates.txt @@ -10,7 +10,7 @@ Every change is expected to have an unique Change-Id. Since this error should never occur in practice, you should inform your Gerrit administrator if you hit this problem and/or -link:http://code.google.com/p/gerrit/issues/list[open a Gerrit issue]. +link:https://bugs.chromium.org/p/gerrit/issues/list[open a Gerrit issue]. In any case to not be blocked with your work, you can simply create a new Change-Id for your commit and then push it as new change to diff --git a/Documentation/index.txt b/Documentation/index.txt index 50770a96c6..63e3be64a8 100644 --- a/Documentation/index.txt +++ b/Documentation/index.txt @@ -79,7 +79,7 @@ * link:licenses.html[Licenses and Notices] * link:https://www.gerritcodereview.com/[Homepage] * link:https://www.gerritcodereview.com/download/index.html[Downloads] -* link:http://code.google.com/p/gerrit/issues/list[Issue Tracking] +* link:https://bugs.chromium.org/p/gerrit/issues/list[Issue Tracking] * link:https://gerrit.googlesource.com/gerrit[Source Code] * link:https://www.gerritcodereview.com/about.md[A History of Gerrit Code Review] diff --git a/README.md b/README.md index f5b929d125..b8de6e27d2 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ There is a mirror of the repository on [Github](https://github.com/gerrit-review ## Reporting bugs -Please report bugs on the [issue tracker](https://code.google.com/p/gerrit/issues/list). +Please report bugs on the [issue tracker](https://bugs.chromium.org/p/gerrit/issues/list). ## Contribute From bd5fdc3d29038058c04b1242f53f68c9f8846fe3 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 7 Dec 2015 16:16:10 -0500 Subject: [PATCH 3/8] Add a testing method to set the clock step used by TimeUtil Change-Id: Ia6e642474c97e5217a6c5de01db41c8d24ba5d51 --- .../gerrit/acceptance/edit/ChangeEditIT.java | 18 +---- .../acceptance/git/AbstractPushForReview.java | 18 +---- .../rest/change/ChangeMessagesIT.java | 20 +---- gerrit-server/BUCK | 3 +- .../notedb/AbstractChangeNotesTest.java | 20 +---- .../change/AbstractQueryChangesTest.java | 44 ++++------- .../change/LuceneQueryChangesV14Test.java | 4 +- .../google/gerrit/testutil/TestTimeUtil.java | 75 +++++++++++++++++++ 8 files changed, 106 insertions(+), 96 deletions(-) create mode 100644 gerrit-server/src/test/java/com/google/gerrit/testutil/TestTimeUtil.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java index 4cd11d16ea..815ea3f988 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java @@ -17,7 +17,6 @@ package com.google.gerrit.acceptance.edit; import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; -import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; import static org.apache.http.HttpStatus.SC_CONFLICT; import static org.apache.http.HttpStatus.SC_FORBIDDEN; @@ -58,6 +57,7 @@ import com.google.gerrit.server.edit.UnchangedCommitMessageException; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.Util; +import com.google.gerrit.testutil.TestTimeUtil; import com.google.gson.stream.JsonReader; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; @@ -66,9 +66,6 @@ import org.apache.commons.codec.binary.StringUtils; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.RefUpdate; -import org.joda.time.DateTime; -import org.joda.time.DateTimeUtils; -import org.joda.time.DateTimeUtils.MillisProvider; import org.junit.After; import org.junit.AfterClass; import org.junit.Before; @@ -81,7 +78,6 @@ import java.util.ArrayList; import java.util.Date; import java.util.Iterator; import java.util.List; -import java.util.concurrent.atomic.AtomicLong; public class ChangeEditIT extends AbstractDaemonTest { @@ -114,20 +110,12 @@ public class ChangeEditIT extends AbstractDaemonTest { @BeforeClass public static void setTimeForTesting() { - final long clockStepMs = MILLISECONDS.convert(1, SECONDS); - final AtomicLong clockMs = new AtomicLong( - new DateTime(2009, 9, 30, 17, 0, 0).getMillis()); - DateTimeUtils.setCurrentMillisProvider(new MillisProvider() { - @Override - public long getMillis() { - return clockMs.getAndAdd(clockStepMs); - } - }); + TestTimeUtil.resetWithClockStep(1, SECONDS); } @AfterClass public static void restoreTime() { - DateTimeUtils.setCurrentMillisSystem(); + TestTimeUtil.useSystemTime(); } @Before diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 86e7ae185a..3f36a2df7e 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -17,7 +17,6 @@ package com.google.gerrit.acceptance.git; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.acceptance.GitUtil.pushHead; -import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; import com.google.common.collect.ImmutableSet; @@ -33,12 +32,10 @@ import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.EditInfo; import com.google.gerrit.extensions.common.LabelInfo; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.testutil.TestTimeUtil; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.transport.PushResult; -import org.joda.time.DateTime; -import org.joda.time.DateTimeUtils; -import org.joda.time.DateTimeUtils.MillisProvider; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; @@ -46,7 +43,6 @@ import org.junit.Test; import java.util.List; import java.util.Set; -import java.util.concurrent.atomic.AtomicLong; public abstract class AbstractPushForReview extends AbstractDaemonTest { protected enum Protocol { @@ -58,20 +54,12 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { @BeforeClass public static void setTimeForTesting() { - final long clockStepMs = MILLISECONDS.convert(1, SECONDS); - final AtomicLong clockMs = new AtomicLong( - new DateTime(2009, 9, 30, 17, 0, 0).getMillis()); - DateTimeUtils.setCurrentMillisProvider(new MillisProvider() { - @Override - public long getMillis() { - return clockMs.getAndAdd(clockStepMs); - } - }); + TestTimeUtil.resetWithClockStep(1, SECONDS); } @AfterClass public static void restoreTime() { - DateTimeUtils.setCurrentMillisSystem(); + TestTimeUtil.useSystemTime(); } @Before diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java index d726d70828..91f2962360 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java @@ -15,7 +15,6 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; -import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; import com.google.gerrit.acceptance.AbstractDaemonTest; @@ -23,41 +22,28 @@ import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeMessageInfo; import com.google.gerrit.testutil.ConfigSuite; +import com.google.gerrit.testutil.TestTimeUtil; -import org.joda.time.DateTime; -import org.joda.time.DateTimeUtils; -import org.joda.time.DateTimeUtils.MillisProvider; import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import java.util.Iterator; -import java.util.concurrent.atomic.AtomicLong; @RunWith(ConfigSuite.class) public class ChangeMessagesIT extends AbstractDaemonTest { private String systemTimeZone; - private volatile long clockStepMs; @Before public void setTimeForTesting() { systemTimeZone = System.setProperty("user.timezone", "US/Eastern"); - clockStepMs = MILLISECONDS.convert(1, SECONDS); - final AtomicLong clockMs = new AtomicLong( - new DateTime(2009, 9, 30, 17, 0, 0).getMillis()); - - DateTimeUtils.setCurrentMillisProvider(new MillisProvider() { - @Override - public long getMillis() { - return clockMs.getAndAdd(clockStepMs); - } - }); + TestTimeUtil.resetWithClockStep(1, SECONDS); } @After public void resetTime() { - DateTimeUtils.setCurrentMillisSystem(); + TestTimeUtil.useSystemTime(); System.setProperty("user.timezone", systemTimeZone); } diff --git a/gerrit-server/BUCK b/gerrit-server/BUCK index c264779c6a..e3b05915d8 100644 --- a/gerrit-server/BUCK +++ b/gerrit-server/BUCK @@ -97,6 +97,7 @@ TESTUTIL_DEPS = [ '//lib/guice:guice-servlet', '//lib/jgit:jgit', '//lib/jgit:junit', + '//lib/joda:joda-time', '//lib/log:api', '//lib/log:impl_log4j', '//lib/log:log4j', @@ -173,7 +174,6 @@ java_test( '//gerrit-common:annotations', '//gerrit-server/src/main/prolog:common', '//lib/antlr:java_runtime', - '//lib/joda:joda-time', ], source_under_test = [':server'], ) @@ -194,7 +194,6 @@ java_test( '//lib:grappa', '//lib:guava', '//lib/guice:guice-assistedinject', - '//lib/joda:joda-time', '//lib/prolog:runtime', ], source_under_test = [':server'], diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java index 8030f9f967..cbd8ff55f2 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java @@ -15,7 +15,6 @@ package com.google.gerrit.server.notedb; import static com.google.inject.Scopes.SINGLETON; -import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; import com.google.common.collect.ImmutableList; @@ -51,6 +50,7 @@ import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.testutil.FakeAccountCache; import com.google.gerrit.testutil.InMemoryRepositoryManager; import com.google.gerrit.testutil.TestChanges; +import com.google.gerrit.testutil.TestTimeUtil; import com.google.gwtorm.client.KeyUtil; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.StandardKeyEncoder; @@ -62,15 +62,11 @@ import com.google.inject.util.Providers; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.PersonIdent; -import org.joda.time.DateTime; -import org.joda.time.DateTimeUtils; -import org.joda.time.DateTimeUtils.MillisProvider; import org.junit.After; import org.junit.Before; import java.sql.Timestamp; import java.util.TimeZone; -import java.util.concurrent.atomic.AtomicLong; public class AbstractChangeNotesTest { private static final TimeZone TZ = @@ -91,7 +87,6 @@ public class AbstractChangeNotesTest { private Injector injector; private String systemTimeZone; - private volatile long clockStepMs; @Inject private AllUsersNameProvider allUsers; @@ -151,21 +146,12 @@ public class AbstractChangeNotesTest { private void setTimeForTesting() { systemTimeZone = System.setProperty("user.timezone", "US/Eastern"); - clockStepMs = MILLISECONDS.convert(1, SECONDS); - final AtomicLong clockMs = new AtomicLong( - new DateTime(2009, 9, 30, 17, 0, 0).getMillis()); - - DateTimeUtils.setCurrentMillisProvider(new MillisProvider() { - @Override - public long getMillis() { - return clockMs.getAndAdd(clockStepMs); - } - }); + TestTimeUtil.resetWithClockStep(1, SECONDS); } @After public void resetTime() { - DateTimeUtils.setCurrentMillisSystem(); + TestTimeUtil.useSystemTime(); System.setProperty("user.timezone", systemTimeZone); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 3b053702e3..1d7a1ae598 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -71,6 +71,7 @@ import com.google.gerrit.testutil.DisabledReviewDb; import com.google.gerrit.testutil.InMemoryDatabase; import com.google.gerrit.testutil.InMemoryRepositoryManager; import com.google.gerrit.testutil.InMemoryRepositoryManager.Repo; +import com.google.gerrit.testutil.TestTimeUtil; import com.google.inject.Inject; import com.google.inject.Injector; import com.google.inject.Provider; @@ -80,9 +81,6 @@ import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.revwalk.RevCommit; -import org.joda.time.DateTime; -import org.joda.time.DateTimeUtils; -import org.joda.time.DateTimeUtils.MillisProvider; import org.junit.After; import org.junit.Before; import org.junit.Ignore; @@ -96,7 +94,6 @@ import java.util.Arrays; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.concurrent.atomic.AtomicLong; @Ignore @RunWith(ConfigSuite.class) @@ -144,7 +141,6 @@ public abstract class AbstractQueryChangesTest { protected ReviewDb db; protected Account.Id userId; protected CurrentUser user; - protected volatile long clockStepMs; private String systemTimeZone; @@ -200,21 +196,12 @@ public abstract class AbstractQueryChangesTest { @Before public void setTimeForTesting() { systemTimeZone = System.setProperty("user.timezone", "US/Eastern"); - clockStepMs = 1; - final AtomicLong clockMs = new AtomicLong( - new DateTime(2009, 9, 30, 17, 0, 0).getMillis()); - - DateTimeUtils.setCurrentMillisProvider(new MillisProvider() { - @Override - public long getMillis() { - return clockMs.getAndAdd(clockStepMs); - } - }); + TestTimeUtil.resetWithClockStep(1, MILLISECONDS); } @After public void resetTime() { - DateTimeUtils.setCurrentMillisSystem(); + TestTimeUtil.useSystemTime(); System.setProperty("user.timezone", systemTimeZone); } @@ -791,7 +778,7 @@ public abstract class AbstractQueryChangesTest { @Test public void updateOrder() throws Exception { - clockStepMs = MILLISECONDS.convert(2, MINUTES); + TestTimeUtil.resetWithClockStep(2, MINUTES); TestRepository repo = createProject("repo"); List inserters = Lists.newArrayList(); List changes = Lists.newArrayList(); @@ -816,7 +803,7 @@ public abstract class AbstractQueryChangesTest { @Test public void updatedOrderWithMinuteResolution() throws Exception { - clockStepMs = MILLISECONDS.convert(2, MINUTES); + TestTimeUtil.resetWithClockStep(2, MINUTES); TestRepository repo = createProject("repo"); ChangeInserter ins1 = newChange(repo, null, null, null, null); Change change1 = insert(ins1); @@ -970,16 +957,17 @@ public abstract class AbstractQueryChangesTest { @Test public void byAge() throws Exception { - long thirtyHours = MILLISECONDS.convert(30, HOURS); - clockStepMs = thirtyHours; + long thirtyHoursInMs = MILLISECONDS.convert(30, HOURS); + TestTimeUtil.resetWithClockStep(thirtyHoursInMs, MILLISECONDS); TestRepository repo = createProject("repo"); Change change1 = insert(newChange(repo, null, null, null, null)); Change change2 = insert(newChange(repo, null, null, null, null)); - clockStepMs = 0; // Queried by AgePredicate constructor. + // Queried by AgePredicate constructor. + TestTimeUtil.setClockStep(0, MILLISECONDS); long now = TimeUtil.nowMs(); assertThat(lastUpdatedMs(change2) - lastUpdatedMs(change1)) - .isEqualTo(thirtyHours); - assertThat(now - lastUpdatedMs(change2)).isEqualTo(thirtyHours); + .isEqualTo(thirtyHoursInMs); + assertThat(now - lastUpdatedMs(change2)).isEqualTo(thirtyHoursInMs); assertThat(TimeUtil.nowMs()).isEqualTo(now); assertQuery("-age:1d"); @@ -993,11 +981,11 @@ public abstract class AbstractQueryChangesTest { @Test public void byBefore() throws Exception { - clockStepMs = MILLISECONDS.convert(30, HOURS); + TestTimeUtil.resetWithClockStep(30, HOURS); TestRepository repo = createProject("repo"); Change change1 = insert(newChange(repo, null, null, null, null)); Change change2 = insert(newChange(repo, null, null, null, null)); - clockStepMs = 0; + TestTimeUtil.setClockStep(0, MILLISECONDS); assertQuery("before:2009-09-29"); assertQuery("before:2009-09-30"); @@ -1013,11 +1001,11 @@ public abstract class AbstractQueryChangesTest { @Test public void byAfter() throws Exception { - clockStepMs = MILLISECONDS.convert(30, HOURS); + TestTimeUtil.resetWithClockStep(30, HOURS); TestRepository repo = createProject("repo"); Change change1 = insert(newChange(repo, null, null, null, null)); Change change2 = insert(newChange(repo, null, null, null, null)); - clockStepMs = 0; + TestTimeUtil.setClockStep(0, MILLISECONDS); assertQuery("after:2009-10-03"); assertQuery("after:\"2009-10-01 20:59:59 -0400\"", change2); @@ -1292,7 +1280,7 @@ public abstract class AbstractQueryChangesTest { @Test public void reviewedBy() throws Exception { - clockStepMs = MILLISECONDS.convert(2, MINUTES); + TestTimeUtil.resetWithClockStep(2, MINUTES); TestRepository repo = createProject("repo"); Change change1 = insert(newChange(repo, null, null, null, null)); Change change2 = insert(newChange(repo, null, null, null, null)); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java index 7e7899b50c..8b22ff594c 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java @@ -15,7 +15,6 @@ package com.google.gerrit.server.query.change; import static com.google.common.truth.Truth.assertThat; -import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.MINUTES; import com.google.gerrit.extensions.api.changes.ReviewInput; @@ -25,6 +24,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.server.account.AuthRequest; import com.google.gerrit.testutil.InMemoryModule; import com.google.gerrit.testutil.InMemoryRepositoryManager.Repo; +import com.google.gerrit.testutil.TestTimeUtil; import com.google.inject.Guice; import com.google.inject.Injector; @@ -80,7 +80,7 @@ public class LuceneQueryChangesV14Test extends LuceneQueryChangesTest { @Test public void isReviewed() throws Exception { - clockStepMs = MILLISECONDS.convert(2, MINUTES); + TestTimeUtil.resetWithClockStep(2, MINUTES); TestRepository repo = createProject("repo"); Change change1 = insert(newChange(repo, null, null, null, null)); Change change2 = insert(newChange(repo, null, null, null, null)); diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestTimeUtil.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestTimeUtil.java new file mode 100644 index 0000000000..4c71c573fc --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestTimeUtil.java @@ -0,0 +1,75 @@ +// Copyright (C) 2015 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.testutil; + +import static com.google.common.base.Preconditions.checkState; +import static java.util.concurrent.TimeUnit.MILLISECONDS; + +import org.joda.time.DateTime; +import org.joda.time.DateTimeUtils; +import org.joda.time.DateTimeUtils.MillisProvider; +import org.joda.time.DateTimeZone; + +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; + +/** Static utility methods for dealing with dates and times in tests. */ +public class TestTimeUtil { + private static Long clockStepMs; + private static AtomicLong clockMs; + + /** + * Reset the clock to a known start point, then set the clock step. + *

+ * The clock is initially set to 2009/09/30 17:00:00 -0400. + * + * @param clockStep amount to increment clock by on each lookup. + * @param clockStepUnit time unit for {@code clockStep}. + */ + public static synchronized void resetWithClockStep( + long clockStep, TimeUnit clockStepUnit) { + // Set an arbitrary start point so tests are more repeatable. + clockMs = new AtomicLong( + new DateTime(2009, 9, 30, 17, 0, 0, DateTimeZone.forOffsetHours(-4)) + .getMillis()); + setClockStep(clockStep, clockStepUnit); + } + + /** + * Set the clock step used by {@link com.google.gerrit.common.TimeUtil}. + * + * @param clockStep amount to increment clock by on each lookup. + * @param clockStepUnit time unit for {@code clockStep}. + */ + public static synchronized void setClockStep( + long clockStep, TimeUnit clockStepUnit) { + checkState(clockMs != null, "call resetWithClockStep first"); + clockStepMs = MILLISECONDS.convert(clockStep, clockStepUnit); + DateTimeUtils.setCurrentMillisProvider(new MillisProvider() { + @Override + public long getMillis() { + return clockMs.getAndAdd(clockStepMs); + } + }); + } + + /** Reset the clock to use the actual system clock. */ + public static synchronized void useSystemTime() { + DateTimeUtils.setCurrentMillisSystem(); + } + + private TestTimeUtil() { + } +} From f8d957ea634cd05522bbb47c0a8a4049949dc8cb Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 7 Dec 2015 15:50:12 -0500 Subject: [PATCH 4/8] CreateChangeIT: Fix flaky test This test was occasionally failing with "Not found" on the new change. After the previous commit, it became clear that this was multiple changes with the same Change-Id (as opposed to a Lucene indexing race, which would have been harder to track down): FAILURE com.google.gerrit.acceptance.rest.change.CreateChangeIT createNewChange: Multiple changes found for I456d43543ea95b7b39ea3ebc5150db2493efc943 com.google.gerrit.extensions.restapi.ResourceNotFoundException: Multiple changes found for I456d43543ea95b7b39ea3ebc5150db2493efc943 at com.google.gerrit.server.change.ChangesCollection.parse(ChangesCollection.java:99) at com.google.gerrit.server.api.changes.ChangesImpl.create(ChangesImpl.java:96) at com.google.gerrit.acceptance.rest.change.CreateChangeIT.assertCreateSucceeds(CreateChangeIT.java:94) at com.google.gerrit.acceptance.rest.change.CreateChangeIT.createNewChange(CreateChangeIT.java:66) The reason for this is that the computed Change-Id in CreateChange only takes into account the current branch tip, subject, and author. In CreateChangeIT, the branch tip is the same (nonexistent) and the subject is a constant, meaning if the tests run fast enough, the timestamp is the same as well. Fix this by setting the current millis provider as in some other tests. Change-Id: Ifc6d478f8bd633a1996c8352869ffb670ca32325 --- .../acceptance/rest/change/CreateChangeIT.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index d0bcfd96b4..9edf9b2d27 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -16,6 +16,7 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; +import static java.util.concurrent.TimeUnit.SECONDS; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; @@ -26,8 +27,11 @@ import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.testutil.ConfigSuite; +import com.google.gerrit.testutil.TestTimeUtil; import org.eclipse.jgit.lib.Config; +import org.junit.AfterClass; +import org.junit.BeforeClass; import org.junit.Test; @NoHttpd @@ -37,6 +41,17 @@ public class CreateChangeIT extends AbstractDaemonTest { return allowDraftsDisabledConfig(); } + @BeforeClass + public static void setTimeForTesting() { + TestTimeUtil.resetWithClockStep(1, SECONDS); + } + + @AfterClass + public static void restoreTime() { + TestTimeUtil.useSystemTime(); + } + + @Test public void createEmptyChange_MissingBranch() throws Exception { ChangeInfo ci = new ChangeInfo(); From 4306a0921df9451839305b8db29be2b662d43f34 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 23 Jun 2016 18:03:00 +0900 Subject: [PATCH 5/8] ChangeIT#commitFooters: Fix setup of test labels - The Verified label was set up with "Passes" as -1 and "Failed" as +1 which was opposite to what one would expect. - The custom labels were added to the config with "verified". Change-Id: I85c5726a988ce1e3951317dba02e4682f830218e --- .../com/google/gerrit/acceptance/api/change/ChangeIT.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 92d7980d5a..23bb1b9531 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -509,15 +509,15 @@ public class ChangeIT extends AbstractDaemonTest { @Test public void commitFooters() throws Exception { LabelType verified = category("Verified", - value(1, "Failed"), value(0, "No score"), value(-1, "Passes")); + value(1, "Passes"), value(0, "No score"), value(-1, "Failed")); LabelType custom1 = category("Custom1", value(1, "Positive"), value(0, "No score"), value(-1, "Negative")); LabelType custom2 = category("Custom2", value(1, "Positive"), value(0, "No score"), value(-1, "Negative")); ProjectConfig cfg = projectCache.checkedGet(project).getConfig(); cfg.getLabelSections().put(verified.getName(), verified); - cfg.getLabelSections().put(custom1.getName(), verified); - cfg.getLabelSections().put(custom2.getName(), verified); + cfg.getLabelSections().put(custom1.getName(), custom1); + cfg.getLabelSections().put(custom2.getName(), custom2); String heads = "refs/heads/*"; AccountGroup.UUID anon = SystemGroupBackend.getGroup(ANONYMOUS_USERS).getUUID(); From 586d0cf011edecb42d529cd65c629e8470eea667 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Thu, 10 Mar 2016 17:40:50 +0100 Subject: [PATCH 6/8] Instead of deleting patch-set-approval set vote to zero Problem: some reviewers get removed from change after it gets submitted. On submitting a change we used to delete those patch-set-approvals where the reviewer didn't have permission to vote in that label or the label didn't exist any more. On the other side, when a reviewer is added to a change this is recorded as patch-set-approval on some label, with zero vote. If the chosen label happens to be one where the reviewer doesn't have permission to vote, this patch-set-approval will be deleted on submit and the reviewer will be removed from the change. Even if the added reviewer did have permission to vote in the chosen label, the label could get deleted before the change is submitted. Again, the reviewer will be removed from the change. Instead of deleting patch-set-approvals set the vote to zero so that we don't accidentally remove reviewers. NOTE: this issue doesn't occur when using NoteDb. In NoteDb we keep track of reviewers explicitly and this is why for NoteDb we still want to delete these approvals. Bug: Issue 4163 Change-Id: I4d4f8d7b55c12a0484257192e7d0ed8d2f71ebcc --- .../com/google/gerrit/server/git/MergeOp.java | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 9a95b520d3..ad1576b173 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.git; import static com.google.common.base.Preconditions.checkState; import static org.eclipse.jgit.lib.RefDatabase.ALL; +import com.google.common.base.Function; import com.google.common.base.Optional; import com.google.common.base.Predicate; import com.google.common.collect.ArrayListMultimap; @@ -1117,7 +1118,7 @@ public class MergeOp { logDebug("Adding submit label " + submit); db.patchSetApprovals().upsert(normalized.getNormalized()); - db.patchSetApprovals().delete(normalized.deleted()); + db.patchSetApprovals().update(zero(normalized.deleted())); try { return saveToBatch(control, update, normalized, timestamp); @@ -1126,6 +1127,20 @@ public class MergeOp { } } + private static Iterable zero( + Iterable approvals) { + return Iterables.transform(approvals, + new Function() { + @Override + public PatchSetApproval apply(PatchSetApproval in) { + PatchSetApproval copy = new PatchSetApproval(in.getPatchSetId(), in); + copy.setValue((short) 0); + return copy; + } + }); + } + + private BatchMetaDataUpdate saveToBatch(ChangeControl ctl, ChangeUpdate callerUpdate, LabelNormalizer.Result normalized, Timestamp timestamp) throws IOException { From 5da67be77395c7d95bc2998fcb99907f8ab56d48 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 23 Jun 2016 13:11:15 +0900 Subject: [PATCH 7/8] ChangeIT: Assert that submitting a change doesn't remove non-voting reviewers If a project is configured with two approval labels, for example Verified and Code-Review, and a user has permission to approve on only one of them, when the change is submitted without the user giving any review, the user should not be removed from the review. Bug: Issue 4163 Change-Id: Iff87398d3fba2ca0b53f0040a1ebd122cc0e079b --- .../acceptance/api/change/ChangeIT.java | 66 +++++++++++++++++++ .../extensions/api/changes/ReviewInput.java | 4 ++ 2 files changed, 70 insertions(+) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 23bb1b9531..d92a887027 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME; import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT; import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; +import static com.google.gerrit.server.group.SystemGroupBackend.CHANGE_OWNER; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static com.google.gerrit.server.project.Util.blockLabel; import static com.google.gerrit.server.project.Util.category; @@ -345,6 +346,71 @@ public class ChangeIT extends AbstractDaemonTest { .containsExactlyElementsIn(ImmutableSet.of(admin.getId(), user.id)); } + @Test + public void nonVotingReviewerStaysAfterSubmit() throws Exception { + LabelType verified = category("Verified", + value(1, "Passes"), value(0, "No score"), value(-1, "Failed")); + ProjectConfig cfg = projectCache.checkedGet(project).getConfig(); + cfg.getLabelSections().put(verified.getName(), verified); + String heads = "refs/heads/*"; + AccountGroup.UUID owners = + SystemGroupBackend.getGroup(CHANGE_OWNER).getUUID(); + AccountGroup.UUID registered = + SystemGroupBackend.getGroup(REGISTERED_USERS).getUUID(); + Util.allow(cfg, + Permission.forLabel(verified.getName()), -1, 1, owners, heads); + Util.allow(cfg, + Permission.forLabel("Code-Review"), -2, +2, registered, heads); + saveProjectConfig(project, cfg); + + // Set Code-Review+2 and Verified+1 as admin (change owner) + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String commit = r.getCommit().name(); + ReviewInput input = ReviewInput.approve(); + input.label(verified.getName(), 1); + gApi.changes() + .id(changeId) + .revision(commit) + .review(input); + + // Reviewers should only be "admin" + assertThat(getReviewers(changeId)) + .containsExactlyElementsIn(ImmutableSet.of(admin.getId())); + + // Add the user as reviewer + AddReviewerInput in = new AddReviewerInput(); + in.reviewer = user.email; + gApi.changes() + .id(changeId) + .addReviewer(in); + assertThat(getReviewers(changeId)) + .containsExactlyElementsIn(ImmutableSet.of(admin.getId(), user.getId())); + + // Approve the change as user, then remove the approval + // (only to confirm that the user does have Code-Review+2 permission) + setApiUser(user); + gApi.changes() + .id(changeId) + .revision(commit) + .review(ReviewInput.approve()); + gApi.changes() + .id(changeId) + .revision(commit) + .review(ReviewInput.noScore()); + + // Submit the change + setApiUser(admin); + gApi.changes() + .id(changeId) + .revision(commit) + .submit(); + + // User should still be on the change + assertThat(getReviewers(changeId)) + .containsExactlyElementsIn(ImmutableSet.of(admin.getId(), user.getId())); + } + @Test public void createEmptyChange() throws Exception { ChangeInfo in = new ChangeInfo(); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java index e6ce6b2ee4..ee043ebafb 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java @@ -122,6 +122,10 @@ public class ReviewInput { return new ReviewInput().label("Code-Review", -1); } + public static ReviewInput noScore() { + return new ReviewInput().label("Code-Review", 0); + } + public static ReviewInput approve() { return new ReviewInput().label("Code-Review", 2); } From ea89a23f3a86390986291084a616fe915173e5a2 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 27 Jun 2016 11:15:42 +0900 Subject: [PATCH 8/8] CreateTag: Remove hook invocation Hooks are invoked from ChangeHooksApiListener. Change-Id: I1dcf439cb8c70c73bb9de36fdccdd8195a9a2551 --- .../java/com/google/gerrit/server/project/CreateTag.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateTag.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateTag.java index 6145362f4a..446fa72ab7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateTag.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateTag.java @@ -18,7 +18,6 @@ import static org.eclipse.jgit.lib.Constants.R_REFS; import static org.eclipse.jgit.lib.Constants.R_TAGS; import com.google.common.base.Strings; -import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.projects.TagInfo; @@ -29,7 +28,6 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; -import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; @@ -65,7 +63,6 @@ public class CreateTag implements RestModifyView { private final GitRepositoryManager repoManager; private final TagCache tagCache; private final GitReferenceUpdated referenceUpdated; - private final ChangeHooks hooks; private String ref; @Inject @@ -73,13 +70,11 @@ public class CreateTag implements RestModifyView { GitRepositoryManager repoManager, TagCache tagCache, GitReferenceUpdated referenceUpdated, - ChangeHooks hooks, @Assisted String ref) { this.identifiedUser = identifiedUser; this.repoManager = repoManager; this.tagCache = tagCache; this.referenceUpdated = referenceUpdated; - this.hooks = hooks; this.ref = ref; } @@ -150,9 +145,6 @@ public class CreateTag implements RestModifyView { referenceUpdated.fire(resource.getNameKey(), ref, ObjectId.zeroId(), result.getObjectId(), identifiedUser.get().getAccount()); - hooks.doRefUpdatedHook(new Branch.NameKey(resource.getNameKey(), ref), - ObjectId.zeroId(), result.getObjectId(), - identifiedUser.get().getAccount()); try (RevWalk w = new RevWalk(repo)) { return ListTags.createTagInfo(result, w); }