Test that creation/last-updated timestamps for checks/checkers are reasonable
To verify timestamps we use a test clock with a clock step of 1 second.
Before creation/updating a check/checker we save the current timestamp
which will be handed out next. This is the timestamp that is expected to
be used for the creation/update.
This approach was already used to verify the creation timestamp for
checks in CreateCheckIT#createCheck(). This change adds similar tests
for getting/updating checks, and for checkers.
It turned out that the creation/update operation for checkers consumed
more than one timestamp:
- 1 timestamp for creating the CheckersUpdate instance (injection of
GerritPersonIdent into NoteDbCheckersUpdate)
- 1 timestamp which was explictly retrieved when CheckerUpdate didn't
specify an update timestamp to use (this was done in 2 places, in
NoteDbCheckersUpdate#updateChecker(CheckerUuid, CheckerUpdate) and
in CheckerConfig#onSave(CommitBuilder), since doing this once in
CheckerConfig#onSave(CommitBuilder) covers both, checker creation and
update, the code for this in
NoteDbCheckersUpdate#updateChecker(CheckerUuid, CheckerUpdate) was
removed)
Instead of explicitly retrieving a timestamp when CheckerUpdate didn't
specify an update timestamp we now use the timestamp from the
CommitBuilder which was populated from the timestamp of the injected
GerritPersonIdent (propagated via MetaDataUpdate).
In addition checks/checkers created by the test API didn't respect the
time settings that were done through TestTimeUtil at test
initialization. This was because CheckOperationsImpl and
CheckerOperationsImpl got CheckUpdate/CheckerUpdate directly injected
instead of injecting Provider<CheckUpdate>/Provider<CheckerUpdate>. This
way the timestamp got fixed when CheckOperationsImpl and
CheckerOperationsImpl got created.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I6c6c58cd35bf96adbde1f3f599215aff02467fef
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
index 5d78aae..8898794 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
@@ -32,6 +32,7 @@
import com.google.gerrit.server.ServerInitiated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -41,7 +42,7 @@
public final class CheckOperationsImpl implements CheckOperations {
private final Checks checks;
- private final ChecksUpdate checksUpdate;
+ private final Provider<ChecksUpdate> checksUpdate;
private final CheckJson.Factory checkJsonFactory;
private final GitRepositoryManager repoManager;
@@ -50,7 +51,7 @@
Checks checks,
GitRepositoryManager repoManager,
CheckJson.Factory checkJsonFactory,
- @ServerInitiated ChecksUpdate checksUpdate) {
+ @ServerInitiated Provider<ChecksUpdate> checksUpdate) {
this.checks = checks;
this.repoManager = repoManager;
this.checkJsonFactory = checkJsonFactory;
@@ -65,7 +66,7 @@
@Override
public TestCheckUpdate.Builder newCheck(CheckKey key) {
return TestCheckUpdate.builder(key)
- .setCheckUpdater(u -> checksUpdate.createCheck(key, toCheckUpdate(u)));
+ .setCheckUpdater(u -> checksUpdate.get().createCheck(key, toCheckUpdate(u)));
}
final class PerCheckOperationsImpl implements PerCheckOperations {
@@ -115,7 +116,8 @@
@Override
public TestCheckUpdate.Builder forUpdate() {
return TestCheckUpdate.builder(key)
- .setCheckUpdater(testUpdate -> checksUpdate.updateCheck(key, toCheckUpdate(testUpdate)));
+ .setCheckUpdater(
+ testUpdate -> checksUpdate.get().updateCheck(key, toCheckUpdate(testUpdate)));
}
}
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
index bbc0386..fe4d54f 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
@@ -39,6 +39,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gwtorm.server.OrmDuplicateKeyException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Optional;
@@ -65,7 +66,7 @@
@Singleton
public class CheckerOperationsImpl implements CheckerOperations {
private final Checkers checkers;
- private final CheckersUpdate checkersUpdate;
+ private final Provider<CheckersUpdate> checkersUpdate;
private final GitRepositoryManager repoManager;
private final AllProjectsName allProjectsName;
private final CheckerJson checkerJson;
@@ -74,7 +75,7 @@
@Inject
public CheckerOperationsImpl(
Checkers checkers,
- @ServerInitiated CheckersUpdate checkersUpdate,
+ @ServerInitiated Provider<CheckersUpdate> checkersUpdate,
GitRepositoryManager repoManager,
AllProjectsName allProjectsName,
CheckerJson checkerJson) {
@@ -100,7 +101,7 @@
throws OrmDuplicateKeyException, ConfigInvalidException, IOException {
CheckerCreation checkerCreation = toCheckerCreation(testCheckerCreation);
CheckerUpdate checkerUpdate = toCheckerUpdate(testCheckerCreation);
- Checker checker = checkersUpdate.createChecker(checkerCreation, checkerUpdate);
+ Checker checker = checkersUpdate.get().createChecker(checkerCreation, checkerUpdate);
return checker.getUuid();
}
@@ -235,7 +236,7 @@
private void updateChecker(TestCheckerUpdate testCheckerUpdate) throws Exception {
CheckerUpdate checkerUpdate = toCheckerUpdate(testCheckerUpdate);
- checkersUpdate.updateChecker(checkerUuid, checkerUpdate);
+ checkersUpdate.get().updateChecker(checkerUuid, checkerUpdate);
if (testCheckerUpdate.forceInvalidConfig()) {
try (Repository repo = repoManager.openRepository(allProjectsName)) {
diff --git a/java/com/google/gerrit/plugins/checks/db/CheckerConfig.java b/java/com/google/gerrit/plugins/checks/db/CheckerConfig.java
index dde68b0..1518ca2 100644
--- a/java/com/google/gerrit/plugins/checks/db/CheckerConfig.java
+++ b/java/com/google/gerrit/plugins/checks/db/CheckerConfig.java
@@ -279,7 +279,9 @@
// for new checkers, we explicitly need to truncate the timestamp here.
Timestamp commitTimestamp =
TimeUtil.truncateToSecond(
- checkerUpdate.flatMap(CheckerUpdate::getUpdatedOn).orElseGet(TimeUtil::nowTs));
+ checkerUpdate
+ .flatMap(CheckerUpdate::getUpdatedOn)
+ .orElseGet(() -> new Timestamp(commit.getCommitter().getWhen().getTime())));
commit.setAuthor(new PersonIdent(commit.getAuthor(), commitTimestamp));
commit.setCommitter(new PersonIdent(commit.getCommitter(), commitTimestamp));
@@ -292,6 +294,8 @@
checkerCreation = Optional.empty();
checkerUpdate = Optional.empty();
+
+ // TODO(ekempin): Don't create a new commit if nothing was changed.
return true;
}
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersUpdate.java b/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersUpdate.java
index 6da5a2a..5c3ccd6 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersUpdate.java
@@ -33,12 +33,10 @@
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryHelper.ActionType;
-import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gwtorm.server.OrmDuplicateKeyException;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
-import java.sql.Timestamp;
import java.util.Optional;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.BatchRefUpdate;
@@ -227,11 +225,6 @@
@Override
public Checker updateChecker(CheckerUuid checkerUuid, CheckerUpdate checkerUpdate)
throws NoSuchCheckerException, IOException, ConfigInvalidException {
- Optional<Timestamp> updatedOn = checkerUpdate.getUpdatedOn();
- if (!updatedOn.isPresent()) {
- updatedOn = Optional.of(TimeUtil.nowTs());
- checkerUpdate = checkerUpdate.toBuilder().setUpdatedOn(updatedOn.get()).build();
- }
return updateCheckerWithRetry(checkerUuid, checkerUpdate);
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
index 5474101..ec24014 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
@@ -37,6 +37,8 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.Inject;
+import java.sql.Timestamp;
+import java.time.Instant;
import java.util.concurrent.TimeUnit;
import org.junit.After;
import org.junit.Before;
@@ -49,6 +51,7 @@
@Before
public void setTimeForTesting() {
TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
+ TestTimeUtil.setClock(Timestamp.from(Instant.EPOCH));
}
@After
@@ -60,6 +63,7 @@
public void createChecker() throws Exception {
Project.NameKey repositoryName = projectOperations.newProject().create();
+ Timestamp expectedCreationTimestamp = TestTimeUtil.getCurrentTimestamp();
CheckerInput input = new CheckerInput();
input.uuid = "test:my-checker";
input.repository = repositoryName.get();
@@ -72,7 +76,7 @@
assertThat(info.status).isEqualTo(CheckerStatus.ENABLED);
assertThat(info.blocking).isEmpty();
assertThat(info.query).isEqualTo("status:open");
- assertThat(info.createdOn).isNotNull();
+ assertThat(info.createdOn).isEqualTo(expectedCreationTimestamp);
assertThat(info.updatedOn).isEqualTo(info.createdOn);
PerCheckerOperations perCheckerOps = checkerOperations.checker(info.uuid);
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
index 3c9d886..e7935cd 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
@@ -35,9 +35,13 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.testing.TestTimeUtil;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.sql.Timestamp;
+import java.time.Instant;
+import java.util.concurrent.TimeUnit;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -48,9 +52,17 @@
@Before
public void setUp() throws Exception {
+ TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
+ TestTimeUtil.setClock(Timestamp.from(Instant.EPOCH));
+
patchSetId = createChange().getPatchSetId();
}
+ @After
+ public void resetTime() {
+ TestTimeUtil.useSystemTime();
+ }
+
@Test
public void getCheckReturnsProject() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
@@ -128,11 +140,10 @@
public void getCheckReturnsCreationTimestamp() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ Timestamp expectedCreationTimestamp = TestTimeUtil.getCurrentTimestamp();
CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
checkOperations.newCheck(checkKey).upsert();
- Timestamp expectedCreationTimestamp = checkOperations.check(checkKey).get().created();
-
assertThat(getCheckInfo(patchSetId, checkerUuid).created).isEqualTo(expectedCreationTimestamp);
assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).created)
.isEqualTo(expectedCreationTimestamp);
@@ -142,11 +153,10 @@
public void getCheckReturnsUpdatedTimestamp() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ Timestamp expectedUpdatedTimestamp = TestTimeUtil.getCurrentTimestamp();
CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
checkOperations.newCheck(checkKey).upsert();
- Timestamp expectedUpdatedTimestamp = checkOperations.check(checkKey).get().updated();
-
assertThat(getCheckInfo(patchSetId, checkerUuid).created).isEqualTo(expectedUpdatedTimestamp);
assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).created)
.isEqualTo(expectedUpdatedTimestamp);
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
index 960e3bc..93f993a 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
@@ -30,15 +30,31 @@
import com.google.gerrit.server.config.ConfigResource;
import com.google.gerrit.server.restapi.config.ListCapabilities;
import com.google.gerrit.server.restapi.config.ListCapabilities.CapabilityInfo;
+import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.Inject;
import java.sql.Timestamp;
+import java.time.Instant;
import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import org.junit.After;
+import org.junit.Before;
import org.junit.Test;
public class GetCheckerIT extends AbstractCheckersTest {
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ListCapabilities listCapabilities;
+ @Before
+ public void setUp() throws Exception {
+ TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
+ TestTimeUtil.setClock(Timestamp.from(Instant.EPOCH));
+ }
+
+ @After
+ public void resetTime() {
+ TestTimeUtil.useSystemTime();
+ }
+
@Test
public void getCheckerReturnsUuid() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().create();
@@ -141,19 +157,17 @@
@Test
public void getCheckerReturnsCreationTimestamp() throws Exception {
+ Timestamp expectedCreationTimestamp = TestTimeUtil.getCurrentTimestamp();
CheckerUuid checkerUuid = checkerOperations.newChecker().create();
- Timestamp expectedCreationTimestamp =
- checkerOperations.checker(checkerUuid).get().getCreatedOn();
assertThat(getCheckerInfo(checkerUuid).createdOn).isEqualTo(expectedCreationTimestamp);
}
@Test
public void getCheckerReturnsUpdatedTimestamp() throws Exception {
+ Timestamp expectedUpdatedTimestamp = TestTimeUtil.getCurrentTimestamp();
CheckerUuid checkerUuid = checkerOperations.newChecker().create();
- Timestamp expectedUpdatedTimestamp =
- checkerOperations.checker(checkerUuid).get().getUpdatedOn();
assertThat(getCheckerInfo(checkerUuid).updatedOn).isEqualTo(expectedUpdatedTimestamp);
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
index 7ae4fac..a671b18 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
@@ -27,7 +27,12 @@
import com.google.gerrit.plugins.checks.api.CheckState;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.Inject;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.util.concurrent.TimeUnit;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -39,6 +44,9 @@
@Before
public void setUp() throws Exception {
+ TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
+ TestTimeUtil.setClock(Timestamp.from(Instant.EPOCH));
+
patchSetId = createChange().getPatchSetId();
CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
@@ -46,6 +54,11 @@
checkOperations.newCheck(checkKey).upsert();
}
+ @After
+ public void resetTime() {
+ TestTimeUtil.useSystemTime();
+ }
+
@Test
public void updateCheckState() throws Exception {
CheckInput input = new CheckInput();
@@ -56,6 +69,27 @@
}
@Test
+ public void updateResultsInNewUpdatedTimestamp() throws Exception {
+ CheckInput input = new CheckInput();
+ input.state = CheckState.FAILED;
+
+ Timestamp expectedUpdateTimestamp = TestTimeUtil.getCurrentTimestamp();
+ CheckInfo info = checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+ assertThat(info.updated).isEqualTo(expectedUpdateTimestamp);
+ }
+
+ @Test
+ public void noOpUpdateDoesntResultInNewUpdatedTimestamp() throws Exception {
+ CheckInput input = new CheckInput();
+ input.state = CheckState.FAILED;
+ CheckInfo info = checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+ Timestamp expectedUpdateTimestamp = info.updated;
+
+ info = checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+ assertThat(info.updated).isEqualTo(expectedUpdateTimestamp);
+ }
+
+ @Test
public void canUpdateCheckForDisabledChecker() throws Exception {
checkerOperations.checker(checkKey.checkerUuid()).forUpdate().disable().update();
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java
index c7ab750..ef45566 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java
@@ -40,6 +40,8 @@
import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.Inject;
+import java.sql.Timestamp;
+import java.time.Instant;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.lib.Config;
@@ -62,6 +64,7 @@
@Before
public void setTimeForTesting() {
TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
+ TestTimeUtil.setClock(Timestamp.from(Instant.EPOCH));
}
@After
@@ -549,6 +552,20 @@
}
@Test
+ public void updateResultsInNewUpdatedTimestamp() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+
+ Timestamp expectedUpdateTimestamp = TestTimeUtil.getCurrentTimestamp();
+ CheckerInput input = new CheckerInput();
+ input.name = "My Checker";
+ CheckerInfo info = checkersApi.id(checkerUuid).update(input);
+ assertThat(info.updatedOn).isEqualTo(expectedUpdateTimestamp);
+ }
+
+ // TODO(ekempin): Add test to verify that a no-op update doesn't create a new updatedOn timestamp
+ // (at the moment it does, but that's a bug)
+
+ @Test
public void updateCheckerWithoutAdministrateCheckersCapabilityFails() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().create();
