Link Checks with Checkers
- Require an enabled checker when creating/updating a check
- Filter out checks where there is no checker or it is disabled/misconfigured
- Add more tests
Change-Id: Ia7a96f5a76b24e7a5e715c062b6ec79e5662ce7b
diff --git a/java/com/google/gerrit/plugins/checks/Checks.java b/java/com/google/gerrit/plugins/checks/Checks.java
index 747d81b..84d19cd 100644
--- a/java/com/google/gerrit/plugins/checks/Checks.java
+++ b/java/com/google/gerrit/plugins/checks/Checks.java
@@ -18,6 +18,7 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gwtorm.server.OrmException;
+import java.io.IOException;
import java.util.List;
import java.util.Optional;
@@ -39,13 +40,14 @@
* @param patchSetId the ID of the patch set
* @return the checks, {@link Optional#empty()} if no checks with the given UUID exists
* @throws OrmException if the checks couldn't be retrieved from the storage
+ * @throws IOException if the checks couldn't be retrieved from the storage
*/
ImmutableList<Check> getChecks(Project.NameKey projectName, PatchSet.Id patchSetId)
- throws OrmException;
+ throws OrmException, IOException;
/**
* Returns a {@link Optional} holding a single check. {@code Optional.empty()} if the check does
* not exist.
*/
- Optional<Check> getCheck(CheckKey checkKey) throws OrmException;
+ Optional<Check> getCheck(CheckKey checkKey) throws OrmException, IOException;
}
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 b289b13..ea7bf03 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
@@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.plugins.checks.Check;
+import com.google.gerrit.plugins.checks.CheckJson;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.CheckUpdate;
import com.google.gerrit.plugins.checks.CheckerRef;
@@ -39,13 +40,18 @@
public final class CheckOperationsImpl implements CheckOperations {
private final Checks checks;
private final ChecksUpdate checksUpdate;
+ private final CheckJson checkJson;
private final GitRepositoryManager repoManager;
@Inject
public CheckOperationsImpl(
- Checks checks, GitRepositoryManager repoManager, @ServerInitiated ChecksUpdate checksUpdate) {
+ Checks checks,
+ GitRepositoryManager repoManager,
+ CheckJson checkJson,
+ @ServerInitiated ChecksUpdate checksUpdate) {
this.checks = checks;
this.repoManager = repoManager;
+ this.checkJson = checkJson;
this.checksUpdate = checksUpdate;
}
@@ -105,8 +111,8 @@
}
@Override
- public CheckInfo asInfo() {
- throw new UnsupportedOperationException("todo");
+ public CheckInfo asInfo() throws Exception {
+ return checkJson.format(get());
}
@Override
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 fbb0994..c16b5a5 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
@@ -233,7 +233,7 @@
CheckerUpdate checkerUpdate = toCheckerUpdate(testCheckerUpdate);
checkersUpdate.updateChecker(checkerUuid, checkerUpdate);
- if (testCheckerUpdate.forceInvalidConfig().orElse(false)) {
+ if (testCheckerUpdate.forceInvalidConfig()) {
try (Repository repo = repoManager.openRepository(allProjectsName)) {
new TestRepository<>(repo)
.branch(CheckerRef.refsCheckers(checkerUuid))
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerUpdate.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerUpdate.java
index 0ee62b3..447a6df 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerUpdate.java
@@ -43,12 +43,14 @@
public abstract Optional<String> query();
- public abstract Optional<Boolean> forceInvalidConfig();
+ public abstract boolean forceInvalidConfig();
abstract ThrowingConsumer<TestCheckerUpdate> checkerUpdater();
public static Builder builder(ThrowingConsumer<TestCheckerUpdate> checkerUpdater) {
- return new AutoValue_TestCheckerUpdate.Builder().checkerUpdater(checkerUpdater);
+ return new AutoValue_TestCheckerUpdate.Builder()
+ .checkerUpdater(checkerUpdater)
+ .forceInvalidConfig(false);
}
@AutoValue.Builder
diff --git a/java/com/google/gerrit/plugins/checks/api/ApiModule.java b/java/com/google/gerrit/plugins/checks/api/ApiModule.java
index 72d94da..7713717 100644
--- a/java/com/google/gerrit/plugins/checks/api/ApiModule.java
+++ b/java/com/google/gerrit/plugins/checks/api/ApiModule.java
@@ -15,6 +15,7 @@
package com.google.gerrit.plugins.checks.api;
import static com.google.gerrit.plugins.checks.api.CheckResource.CHECK_KIND;
+import static com.google.gerrit.plugins.checks.api.CheckerResource.CHECKER_KIND;
import static com.google.gerrit.server.change.RevisionResource.REVISION_KIND;
import com.google.gerrit.extensions.config.FactoryModule;
@@ -29,10 +30,18 @@
public class ApiModule extends AbstractModule {
@Override
protected void configure() {
+ bind(CheckersCollection.class);
+ bind(Checkers.class).to(CheckersImpl.class);
+
install(
new RestApiModule() {
@Override
public void configure() {
+ DynamicMap.mapOf(binder(), CHECKER_KIND);
+ postOnCollection(CHECKER_KIND).to(CreateChecker.class);
+ get(CHECKER_KIND).to(GetChecker.class);
+ post(CHECKER_KIND).to(UpdateChecker.class);
+
DynamicMap.mapOf(binder(), CHECK_KIND);
child(REVISION_KIND, "checks").to(ChecksCollection.class);
postOnCollection(CHECK_KIND).to(PostCheck.class);
@@ -44,6 +53,7 @@
new FactoryModule() {
@Override
public void configure() {
+ factory(CheckerApiImpl.Factory.class);
factory(CheckApiImpl.Factory.class);
factory(ChecksImpl.Factory.class);
}
diff --git a/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java b/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
index 88a019b..b0edeec 100644
--- a/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
+++ b/java/com/google/gerrit/plugins/checks/api/ChecksImpl.java
@@ -44,6 +44,7 @@
private final Checks checks;
private final Provider<ChecksUpdate> checksUpdate;
+ private final Checkers checkers;
private final CheckJson checkJson;
private final CheckApiImpl.Factory checkApiImplFactory;
private final RevisionResource revisionResource;
@@ -53,11 +54,13 @@
Checks checks,
CheckApiImpl.Factory checkApiImplFactory,
CheckJson checkJson,
+ Checkers checkers,
@UserInitiated Provider<ChecksUpdate> checksUpdate,
@Assisted RevisionResource revisionResource) {
this.checks = checks;
this.checkApiImplFactory = checkApiImplFactory;
this.checkJson = checkJson;
+ this.checkers = checkers;
this.checksUpdate = checksUpdate;
this.revisionResource = revisionResource;
}
@@ -67,10 +70,12 @@
if (checkerUuid == null || checkerUuid.isEmpty()) {
throw new BadRequestException("checkerUuid is required");
}
+ // Check that the checker exists and throw a RestApiException if not.
+ CheckerInfo checker = checkers.id(checkerUuid).get();
CheckKey checkKey =
CheckKey.create(
- revisionResource.getProject(), revisionResource.getPatchSet().getId(), checkerUuid);
+ revisionResource.getProject(), revisionResource.getPatchSet().getId(), checker.uuid);
Optional<Check> check = checks.getCheck(checkKey);
return checkApiImplFactory.create(
new CheckResource(
@@ -89,12 +94,12 @@
if (input.state == null) {
throw new BadRequestException("state is required");
}
+ // Check that the checker exists and throw a BadRequestException if not.
+ CheckerInfo checker = checkers.id(input.checkerUuid).get();
CheckKey checkKey =
CheckKey.create(
- revisionResource.getProject(),
- revisionResource.getPatchSet().getId(),
- input.checkerUuid);
+ revisionResource.getProject(), revisionResource.getPatchSet().getId(), checker.uuid);
CheckUpdate checkUpdate =
CheckUpdate.builder()
.setState(input.state)
@@ -111,7 +116,7 @@
return getChecks().stream().map(checkJson::format).collect(toImmutableList());
}
- private List<Check> getChecks() throws IOException, OrmException {
+ private List<Check> getChecks() throws OrmException, IOException {
return checks.getChecks(revisionResource.getProject(), revisionResource.getPatchSet().getId());
}
}
diff --git a/java/com/google/gerrit/plugins/checks/api/HttpModule.java b/java/com/google/gerrit/plugins/checks/api/HttpModule.java
index 92b9e30..f48ec72 100644
--- a/java/com/google/gerrit/plugins/checks/api/HttpModule.java
+++ b/java/com/google/gerrit/plugins/checks/api/HttpModule.java
@@ -14,10 +14,6 @@
package com.google.gerrit.plugins.checks.api;
-import static com.google.gerrit.plugins.checks.api.CheckerResource.CHECKER_KIND;
-
-import com.google.gerrit.extensions.config.FactoryModule;
-import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.RestApiModule;
import com.google.inject.servlet.ServletModule;
@@ -25,10 +21,6 @@
@Override
protected void configureServlets() {
- bind(CheckersCollection.class);
-
- bind(Checkers.class).to(CheckersImpl.class);
-
serveRegex("^/checkers/(.*)$").with(CheckersRestApiServlet.class);
install(
@@ -36,18 +28,7 @@
@Override
public void configure() {
// Checkers
- DynamicMap.mapOf(binder(), CHECKER_KIND);
- postOnCollection(CHECKER_KIND).to(CreateChecker.class);
- get(CHECKER_KIND).to(GetChecker.class);
- post(CHECKER_KIND).to(UpdateChecker.class);
- }
- });
- install(
- new FactoryModule() {
- @Override
- public void configure() {
- factory(CheckerApiImpl.Factory.class);
}
});
}
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
index 0f846c2..1bc0195 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
@@ -15,11 +15,18 @@
package com.google.gerrit.plugins.checks.db;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.Checker;
+import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.PatchSetUtil;
@@ -27,34 +34,42 @@
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.io.IOException;
import java.util.Optional;
+import java.util.Set;
import java.util.stream.Stream;
+import org.eclipse.jgit.errors.ConfigInvalidException;
/** Class to read checks from NoteDb. */
@Singleton
class NoteDbChecks implements Checks {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
private final ChangeNotes.Factory changeNotesFactory;
private final PatchSetUtil psUtil;
private final CheckNotes.Factory checkNotesFactory;
+ private final Checkers checkers;
@Inject
NoteDbChecks(
ChangeNotes.Factory changeNotesFactory,
PatchSetUtil psUtil,
- CheckNotes.Factory checkNotesFactory) {
+ CheckNotes.Factory checkNotesFactory,
+ Checkers checkers) {
this.changeNotesFactory = changeNotesFactory;
this.psUtil = psUtil;
this.checkNotesFactory = checkNotesFactory;
+ this.checkers = checkers;
}
@Override
public ImmutableList<Check> getChecks(Project.NameKey projectName, PatchSet.Id psId)
- throws OrmException {
+ throws OrmException, IOException {
return getChecksAsStream(projectName, psId).collect(toImmutableList());
}
@Override
- public Optional<Check> getCheck(CheckKey checkKey) throws OrmException {
+ public Optional<Check> getCheck(CheckKey checkKey) throws OrmException, IOException {
// TODO(gerrit-team): Instead of reading the complete notes map, read just one note.
return getChecksAsStream(checkKey.project(), checkKey.patchSet())
.filter(c -> c.key().checkerUuid().equals(checkKey.checkerUuid()))
@@ -62,18 +77,38 @@
}
private Stream<Check> getChecksAsStream(Project.NameKey projectName, PatchSet.Id psId)
- throws OrmException {
+ throws OrmException, IOException {
// TODO(gerrit-team): Instead of reading the complete notes map, read just one note.
ChangeNotes notes = changeNotesFactory.create(projectName, psId.getParentKey());
PatchSet patchSet = psUtil.get(notes, psId);
CheckNotes checkNotes = checkNotesFactory.create(notes.getChange());
+
checkNotes.load();
+ Set<CheckerUuid> checkerUuids = activeAndValidCheckersForProject(projectName);
return checkNotes
.getChecks()
.getOrDefault(patchSet.getRevision(), NoteDbCheckMap.empty())
.checks
.entrySet()
.stream()
- .map(e -> e.getValue().toCheck(projectName, psId, e.getKey()));
+ .map(e -> e.getValue().toCheck(projectName, psId, e.getKey()))
+ .filter(c -> checkerUuids.contains(CheckerUuid.parse(c.key().checkerUuid())));
+ }
+
+ /** Get all checkers that apply to a project. Might return a superset of checkers that apply. */
+ private ImmutableSet<CheckerUuid> activeAndValidCheckersForProject(Project.NameKey projectName)
+ throws IOException {
+ Stream<Checker> checkerStream;
+ try {
+ checkerStream = checkers.checkersOf(projectName).stream();
+ } catch (ConfigInvalidException e) {
+ logger.atInfo().withCause(e).log(
+ "can't bulk-load checkers for " + projectName + " will do one-by-one");
+ checkerStream = checkers.listCheckers().stream();
+ }
+ return checkerStream
+ .filter(c -> c.getStatus() == CheckerStatus.ENABLED)
+ .map(Checker::getUuid)
+ .collect(toImmutableSet());
}
}
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
index e74d51e..2701052 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
@@ -25,8 +25,11 @@
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.CheckUpdate;
+import com.google.gerrit.plugins.checks.Checker;
import com.google.gerrit.plugins.checks.CheckerRef;
+import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.ChecksUpdate;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
@@ -72,6 +75,7 @@
private final RetryHelper retryHelper;
private final ChangeNoteUtil noteUtil;
private final Optional<IdentifiedUser> currentUser;
+ private final Checkers checkers;
@AssistedInject
NoteDbChecksUpdate(
@@ -79,8 +83,10 @@
GitReferenceUpdated gitRefUpdated,
RetryHelper retryHelper,
ChangeNoteUtil noteUtil,
+ Checkers checkers,
@GerritPersonIdent PersonIdent personIdent) {
- this(repoManager, gitRefUpdated, retryHelper, noteUtil, personIdent, Optional.empty());
+ this(
+ repoManager, gitRefUpdated, retryHelper, noteUtil, checkers, personIdent, Optional.empty());
}
@AssistedInject
@@ -89,9 +95,17 @@
GitReferenceUpdated gitRefUpdated,
RetryHelper retryHelper,
ChangeNoteUtil noteUtil,
+ Checkers checkers,
@GerritPersonIdent PersonIdent personIdent,
@Assisted IdentifiedUser currentUser) {
- this(repoManager, gitRefUpdated, retryHelper, noteUtil, personIdent, Optional.of(currentUser));
+ this(
+ repoManager,
+ gitRefUpdated,
+ retryHelper,
+ noteUtil,
+ checkers,
+ personIdent,
+ Optional.of(currentUser));
}
private NoteDbChecksUpdate(
@@ -99,12 +113,14 @@
GitReferenceUpdated gitRefUpdated,
RetryHelper retryHelper,
ChangeNoteUtil noteUtil,
+ Checkers checkers,
@GerritPersonIdent PersonIdent personIdent,
Optional<IdentifiedUser> currentUser) {
this.repoManager = repoManager;
this.gitRefUpdated = gitRefUpdated;
this.retryHelper = retryHelper;
this.noteUtil = noteUtil;
+ this.checkers = checkers;
this.currentUser = currentUser;
this.personIdent = personIdent;
}
@@ -141,6 +157,8 @@
private Check upsertCheckInNoteDb(CheckKey checkKey, CheckUpdate checkUpdate, Operation operation)
throws IOException, ConfigInvalidException, OrmDuplicateKeyException {
+ assertCheckerPresentAndEnabled(checkKey.checkerUuid());
+
try (Repository repo = repoManager.openRepository(checkKey.project());
ObjectInserter objectInserter = repo.newObjectInserter();
RevWalk rw = new RevWalk(repo)) {
@@ -181,6 +199,17 @@
}
}
+ private void assertCheckerPresentAndEnabled(String checkerUuid)
+ throws ConfigInvalidException, IOException {
+ Checker checker =
+ checkers
+ .getChecker(checkerUuid)
+ .orElseThrow(() -> new IOException(checkerUuid + " missing"));
+ if (checker.getStatus() != CheckerStatus.ENABLED) {
+ throw new IOException("checker " + checkerUuid + " unknown");
+ }
+ }
+
private boolean updateNotesMap(
CheckKey checkKey,
CheckUpdate checkUpdate,
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/ChecksRestApiBindingsIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/ChecksRestApiBindingsIT.java
index c708c16..96886ed 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/ChecksRestApiBindingsIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/ChecksRestApiBindingsIT.java
@@ -61,7 +61,7 @@
@Test
public void scopedCheckEndpoints() throws Exception {
- CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
CheckKey key = CheckKey.create(project, createChange().getPatchSetId(), checkerUuid.toString());
checkOperations.newCheck(key).setState(CheckState.RUNNING).upsert();
RestApiCallHelper.execute(
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/GetCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/GetCheckIT.java
index ac5843d..2124aa2 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/GetCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/GetCheckIT.java
@@ -17,7 +17,9 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.api.CheckInfo;
import com.google.gerrit.plugins.checks.api.CheckState;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -35,17 +37,38 @@
@Test
public void getCheck() throws Exception {
- String name = "my-checker";
- CheckKey key = CheckKey.create(project, patchSetId, name);
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey key = CheckKey.create(project, patchSetId, checkerUuid.toString());
checkOperations.newCheck(key).setState(CheckState.RUNNING).upsert();
- CheckInfo info = checksApiFactory.revision(patchSetId).id(name).get();
- assertThat(info.checkerUuid).isEqualTo(name);
- assertThat(info.state).isEqualTo(CheckState.RUNNING);
- assertThat(info.started).isNull();
- assertThat(info.finished).isNull();
- assertThat(info.created).isNotNull();
- assertThat(info.updated).isNotNull();
+ CheckInfo info = checksApiFactory.revision(patchSetId).id(checkerUuid.toString()).get();
+ assertThat(info).isEqualTo(checkOperations.check(key).asInfo());
+ }
+
+ @Test
+ public void getCheckForDisabledCheckerThrowsNotFound() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey key = CheckKey.create(project, patchSetId, checkerUuid.toString());
+ checkOperations.newCheck(key).setState(CheckState.RUNNING).upsert();
+
+ checkerOperations.checker(checkerUuid).forUpdate().disable().update();
+
+ exception.expect(ResourceNotFoundException.class);
+ exception.expectMessage("Not found: " + checkerUuid);
+ checksApiFactory.revision(patchSetId).id(checkerUuid.toString()).get();
+ }
+
+ @Test
+ public void getCheckForInvalidCheckerThrowsNotFound() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ CheckKey key = CheckKey.create(project, patchSetId, checkerUuid.toString());
+ checkOperations.newCheck(key).setState(CheckState.RUNNING).upsert();
+
+ checkerOperations.checker(checkerUuid).forUpdate().forceInvalidConfig().update();
+
+ exception.expect(RestApiException.class);
+ exception.expectMessage("Cannot retrieve checker " + checkerUuid);
+ checksApiFactory.revision(patchSetId).id(checkerUuid.toString()).get();
}
@Test
@@ -55,4 +78,12 @@
checksApiFactory.revision(patchSetId).id("non-existing").get();
}
+
+ @Test
+ public void getNonExistingCheckWithInvalidUuidFails() throws Exception {
+ exception.expect(ResourceNotFoundException.class);
+ exception.expectMessage("Not found: n0n-e#isting");
+
+ checksApiFactory.revision(patchSetId).id("n0n-e#isting").get();
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
index 7291a69..77550be 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
import com.google.gerrit.plugins.checks.acceptance.testsuite.CheckOperations.PerCheckOperations;
import com.google.gerrit.plugins.checks.api.CheckInfo;
@@ -53,38 +54,40 @@
@Test
public void createCheck() throws Exception {
- String checkerUuid = "my-checker";
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
CheckInput input = new CheckInput();
- input.checkerUuid = checkerUuid;
+ input.checkerUuid = checkerUuid.toString();
input.state = CheckState.RUNNING;
CheckInfo info = checksApiFactory.revision(patchSetId).create(input).get();
- assertThat(info.checkerUuid).isEqualTo(checkerUuid);
+ assertThat(info.checkerUuid).isEqualTo(checkerUuid.toString());
assertThat(info.state).isEqualTo(CheckState.RUNNING);
assertThat(info.started).isNull();
assertThat(info.finished).isNull();
assertThat(info.created).isNotNull();
assertThat(info.updated).isNotNull();
- CheckKey key = CheckKey.create(project, patchSetId, checkerUuid);
+ CheckKey key = CheckKey.create(project, patchSetId, checkerUuid.toString());
PerCheckOperations perCheckOps = checkOperations.check(key);
// TODO(gerrit-team) Add a Truth subject for the notes map
Map<RevId, String> notes = perCheckOps.notesAsText();
- assertThat(notes).containsExactly(revId, noteDbContent());
+ assertThat(notes).containsExactly(revId, noteDbContent(checkerUuid.toString()));
}
// TODO(gerrit-team) More tests, especially for multiple checkers and PS and how commits behave
- private String noteDbContent() {
+ private String noteDbContent(String uuid) {
return ""
+ "{\n"
+ " \"checks\": {\n"
- + " \"my-checker\": {\n"
+ + " \""
+ + uuid
+ + "\": {\n"
+ " \"state\": \"RUNNING\",\n"
- + " \"created\": \"1970-01-01T00:00:22Z\",\n"
- + " \"updated\": \"1970-01-01T00:00:22Z\"\n"
+ + " \"created\": \"1970-01-01T00:00:25Z\",\n"
+ + " \"updated\": \"1970-01-01T00:00:25Z\"\n"
+ " }\n"
+ " }\n"
+ "}";
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
index cf69ca1..9f4b267 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
@@ -21,15 +21,11 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.plugins.checks.CheckerRef;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
import com.google.gerrit.plugins.checks.api.CheckerInfo;
-import com.google.gerrit.plugins.checks.db.CheckerConfig;
import com.google.inject.Inject;
import java.util.List;
-import org.eclipse.jgit.junit.TestRepository;
-import org.eclipse.jgit.lib.Repository;
import org.junit.Test;
public class ListCheckersIT extends AbstractCheckersTest {
@@ -80,19 +76,10 @@
public void listIgnoresInvalidCheckers() throws Exception {
CheckerUuid checkerUuid =
checkerOperations.newChecker().name("checker-with-name-only").create();
- createInvalidChecker();
+ CheckerUuid invalidCheckerUuid = checkerOperations.newChecker().create();
+ checkerOperations.checker(invalidCheckerUuid).forUpdate().forceInvalidConfig().update();
List<CheckerInfo> allCheckers = checkersApi.all();
assertThat(allCheckers).containsExactly(checkerOperations.checker(checkerUuid).asInfo());
}
-
- private void createInvalidChecker() throws Exception {
- try (Repository repo = repoManager.openRepository(allProjects)) {
- new TestRepository<>(repo)
- .branch(CheckerRef.refsCheckers(CheckerUuid.parse("test:checker")))
- .commit()
- .add(CheckerConfig.CHECKER_CONFIG_FILE, "invalid-config")
- .create();
- }
- }
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
index 1555e20..2a1c4ae 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
import com.google.gerrit.plugins.checks.api.CheckInfo;
import com.google.gerrit.plugins.checks.api.CheckState;
@@ -27,22 +28,44 @@
public class ListChecksIT extends AbstractCheckersTest {
private PatchSet.Id patchSetId;
+ private CheckKey checkKey1;
+ private CheckKey checkKey2;
@Before
public void setTimeForTesting() throws Exception {
patchSetId = createChange().getPatchSetId();
+
+ CheckerUuid checker1Uuid = checkerOperations.newChecker().repository(project).create();
+ CheckerUuid checker2Uuid = checkerOperations.newChecker().repository(project).create();
+
+ checkKey1 = CheckKey.create(project, patchSetId, checker1Uuid.toString());
+ checkOperations.newCheck(checkKey1).setState(CheckState.RUNNING).upsert();
+
+ checkKey2 = CheckKey.create(project, patchSetId, checker2Uuid.toString());
+ checkOperations.newCheck(checkKey2).setState(CheckState.RUNNING).upsert();
}
@Test
public void listAll() throws Exception {
- CheckKey key1 = CheckKey.create(project, patchSetId, "my-checker-1");
- checkOperations.newCheck(key1).setState(CheckState.RUNNING).upsert();
-
- CheckKey key2 = CheckKey.create(project, patchSetId, "my-checker-2");
- checkOperations.newCheck(key2).setState(CheckState.RUNNING).upsert();
-
- // TODO(gerrit-team): Use a truth subject to make this assertation proper
Collection<CheckInfo> info = checksApiFactory.revision(patchSetId).list();
- assertThat(info).hasSize(2);
+ assertThat(info)
+ .containsExactly(
+ checkOperations.check(checkKey1).asInfo(), checkOperations.check(checkKey2).asInfo());
+ }
+
+ @Test
+ public void listExcludesCheckFromDisabledChecker() throws Exception {
+ checkerOperations.checker(checkKey2.checkerUuid()).forUpdate().disable().update();
+
+ Collection<CheckInfo> info = checksApiFactory.revision(patchSetId).list();
+ assertThat(info).containsExactly(checkOperations.check(checkKey1).asInfo());
+ }
+
+ @Test
+ public void listExcludesCheckFromInvalidChecker() throws Exception {
+ checkerOperations.checker(checkKey2.checkerUuid()).forUpdate().forceInvalidConfig().update();
+
+ Collection<CheckInfo> info = checksApiFactory.revision(patchSetId).list();
+ assertThat(info).containsExactly(checkOperations.check(checkKey1).asInfo());
}
}
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 768dfd9..6b5db88 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
@@ -16,7 +16,10 @@
import static com.google.common.truth.Truth.assertThat;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
import com.google.gerrit.plugins.checks.api.CheckInfo;
import com.google.gerrit.plugins.checks.api.CheckInput;
@@ -27,21 +30,48 @@
public class UpdateCheckIT extends AbstractCheckersTest {
private PatchSet.Id patchSetId;
+ private CheckKey checkKey;
@Before
public void setTimeForTesting() throws Exception {
patchSetId = createChange().getPatchSetId();
+
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkKey = CheckKey.create(project, patchSetId, checkerUuid.toString());
+ checkOperations.newCheck(checkKey).setState(CheckState.RUNNING).upsert();
}
@Test
public void updateCheckState() throws Exception {
- CheckKey key = CheckKey.create(project, patchSetId, "my-checker-1");
- checkOperations.newCheck(key).setState(CheckState.RUNNING).upsert();
-
CheckInput input = new CheckInput();
input.state = CheckState.FAILED;
- CheckInfo info = checksApiFactory.revision(patchSetId).id("my-checker-1").update(input);
+ CheckInfo info = checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
assertThat(info.state).isEqualTo(CheckState.FAILED);
}
+
+ @Test
+ public void updateCheckStateForDisabledChecker() throws Exception {
+ checkerOperations.checker(checkKey.checkerUuid()).forUpdate().disable().update();
+
+ exception.expect(ResourceNotFoundException.class);
+ exception.expectMessage("Not found: " + checkKey.checkerUuid());
+ checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(new CheckInput());
+ }
+
+ @Test
+ public void updateCheckStateForInvalidChecker() throws Exception {
+ checkerOperations.checker(checkKey.checkerUuid()).forUpdate().forceInvalidConfig().update();
+
+ exception.expect(RestApiException.class);
+ exception.expectMessage("Cannot retrieve checker " + checkKey.checkerUuid());
+ checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(new CheckInput());
+ }
+
+ @Test
+ public void updateCheckStateWithInvalidUuid() throws Exception {
+ exception.expect(ResourceNotFoundException.class);
+ exception.expectMessage("Not found: in#alid");
+ checksApiFactory.revision(patchSetId).id("in#alid").update(new CheckInput());
+ }
}
