Merge "Add metric to record code owner config validations"
diff --git a/BUILD b/BUILD
index 7c7f9e0..5333141 100644
--- a/BUILD
+++ b/BUILD
@@ -9,9 +9,6 @@
"//tools/bzl:plugin.bzl",
"gerrit_plugin",
)
-load("//tools/bzl:genrule2.bzl", "genrule2")
-load("//tools/bzl:js.bzl", "polygerrit_plugin")
-load("@npm//@bazel/rollup:index.bzl", "rollup_bundle")
gerrit_plugin(
name = "code-owners",
@@ -22,7 +19,7 @@
"Gerrit-HttpModule: com.google.gerrit.plugins.codeowners.module.HttpModule",
"Gerrit-BatchModule: com.google.gerrit.plugins.codeowners.module.BatchModule",
],
- resource_jars = [":code-owners-fe-static"],
+ resource_jars = ["//plugins/code-owners/ui:code-owners"],
resource_strip_prefix = "plugins/code-owners/resources",
resources = glob(["resources/**/*"]),
deps = [
@@ -33,35 +30,3 @@
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/validation",
],
)
-
-polygerrit_plugin(
- name = "code-owners-fe",
- app = "plugin-bundle.js",
- plugin_name = "code-owners",
-)
-
-rollup_bundle(
- name = "plugin-bundle",
- srcs = glob([
- "ui/**/*.js",
- ]),
- entry_point = "ui/plugin.js",
- format = "iife",
- rollup_bin = "//tools/node_tools:rollup-bin",
- sourcemap = "hidden",
- deps = [
- "@tools_npm//rollup-plugin-node-resolve",
- ],
-)
-
-genrule2(
- name = "code-owners-fe-static",
- srcs = [":code-owners-fe"],
- outs = ["code-owners-fe-static.jar"],
- cmd = " && ".join([
- "mkdir $$TMP/static",
- "cp -r $(locations :code-owners-fe) $$TMP/static",
- "cd $$TMP",
- "zip -Drq $$ROOT/$@ -g .",
- ]),
-)
diff --git a/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java
index cd8aacb..12aebb4 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java
@@ -16,6 +16,7 @@
import com.google.gerrit.extensions.restapi.NotImplementedException;
import com.google.gerrit.extensions.restapi.RestApiException;
+import java.util.Optional;
/**
* Java API for change code owners.
@@ -40,6 +41,39 @@
* <p>Allows to set parameters on the request before executing it by calling {@link #get()}.
*/
abstract class CodeOwnerStatusRequest {
+ private Integer start;
+ private Integer limit;
+
+ /**
+ * Sets a limit on the number of code owner statuses that should be returned.
+ *
+ * @param start number of code owner statuses to skip
+ */
+ public CodeOwnerStatusRequest withStart(int start) {
+ this.start = start;
+ return this;
+ }
+
+ /** Returns the number of code owner statuses to skip. */
+ public Optional<Integer> getStart() {
+ return Optional.ofNullable(start);
+ }
+
+ /**
+ * Sets a limit on the number of code owner statuses that should be returned.
+ *
+ * @param limit the limit
+ */
+ public CodeOwnerStatusRequest withLimit(int limit) {
+ this.limit = limit;
+ return this;
+ }
+
+ /** Returns the limit. */
+ public Optional<Integer> getLimit() {
+ return Optional.ofNullable(limit);
+ }
+
/**
* Executes this request and retrieves the code owner status.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerStatusInfo.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerStatusInfo.java
index b10e2be..e9ae34d 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerStatusInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerStatusInfo.java
@@ -34,4 +34,11 @@
/** List of the code owner statuses for the files in the change. */
public List<FileCodeOwnerStatusInfo> fileCodeOwnerStatuses;
+
+ /**
+ * Whether the request would deliver more results if not limited.
+ *
+ * <p>Not set if {@code false}.
+ */
+ public Boolean more;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/ChangeCodeOwnersImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/ChangeCodeOwnersImpl.java
index 5a20f34..17c78cf 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/ChangeCodeOwnersImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/ChangeCodeOwnersImpl.java
@@ -26,6 +26,7 @@
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.restapi.change.Revisions;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
/** Implementation of the {@link ChangeCodeOwners} API. */
@@ -37,17 +38,17 @@
private final Revisions revisions;
private final RevisionCodeOwnersImpl.Factory revisionCodeOwnersApi;
private final ChangeResource changeResource;
- private final GetCodeOwnerStatus getCodeOwnerStatus;
+ private final Provider<GetCodeOwnerStatus> getCodeOwnerStatusProvider;
@Inject
public ChangeCodeOwnersImpl(
Revisions revisions,
RevisionCodeOwnersImpl.Factory revisionCodeOwnersApi,
- GetCodeOwnerStatus getCodeOwnerStatus,
+ Provider<GetCodeOwnerStatus> getCodeOwnerStatusProvider,
@Assisted ChangeResource changeResource) {
this.revisions = revisions;
this.revisionCodeOwnersApi = revisionCodeOwnersApi;
- this.getCodeOwnerStatus = getCodeOwnerStatus;
+ this.getCodeOwnerStatusProvider = getCodeOwnerStatusProvider;
this.changeResource = changeResource;
}
@@ -57,6 +58,9 @@
@Override
public CodeOwnerStatusInfo get() throws RestApiException {
try {
+ GetCodeOwnerStatus getCodeOwnerStatus = getCodeOwnerStatusProvider.get();
+ getStart().ifPresent(getCodeOwnerStatus::setStart);
+ getLimit().ifPresent(getCodeOwnerStatus::setLimit);
return getCodeOwnerStatus.apply(changeResource).value();
} catch (Exception e) {
throw asRestApiException("Cannot get code owner status", e);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index f03d337..2835935 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -14,12 +14,16 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Project;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
@@ -28,16 +32,22 @@
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.InMemoryInserter;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.patch.AutoMerger;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
+import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.List;
+import java.util.Map;
+import java.util.stream.Stream;
import org.eclipse.jgit.diff.DiffEntry;
import org.eclipse.jgit.diff.DiffFormatter;
import org.eclipse.jgit.diff.RawTextComparator;
@@ -51,12 +61,21 @@
import org.eclipse.jgit.util.io.DisabledOutputStream;
/**
- * Class to compute the files that have been changed in a revision.
+ * Class to get/compute the files that have been changed in a revision.
*
- * <p>The file diff is newly computed on each access and not retrieved from any cache. This is
- * better than using {@link com.google.gerrit.server.patch.PatchListCache} which does a lot of
- * unneeded computations and hence is slower. The Gerrit diff caches are currently being redesigned.
- * Once the envisioned {@code ModifiedFilesCache} is available we should consider using it.
+ * <p>The {@link #getFromDiffCache(Project.NameKey, ObjectId)} method is retrieving the file diff
+ * from the diff cache and has rename detection enabled.
+ *
+ * <p>In contrast to this, for the {@code compute} methods the file diff is newly computed on each
+ * access and rename detection is disabled (as it's too expensive to do it on each access).
+ *
+ * <p>If possible, using {@link #getFromDiffCache(Project.NameKey, ObjectId)} is preferred, however
+ * {@link #getFromDiffCache(Project.NameKey, ObjectId)} cannot be used for newly created commits
+ * that are only available from a specific {@link RevWalk} instance since the {@link RevWalk}
+ * instance cannot be passed in.
+ *
+ * <p>The {@link com.google.gerrit.server.patch.PatchListCache} is deprecated, and hence it not
+ * being used here.
*/
@Singleton
public class ChangedFiles {
@@ -66,29 +85,64 @@
private final GitRepositoryManager repoManager;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
+ private final DiffOperations diffOperations;
private final Provider<AutoMerger> autoMergerProvider;
private final CodeOwnerMetrics codeOwnerMetrics;
private final ThreeWayMergeStrategy mergeStrategy;
+ private final ExperimentFeatures experimentFeatures;
@Inject
public ChangedFiles(
@GerritServerConfig Config cfg,
GitRepositoryManager repoManager,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
+ DiffOperations diffOperations,
Provider<AutoMerger> autoMergerProvider,
- CodeOwnerMetrics codeOwnerMetrics) {
+ CodeOwnerMetrics codeOwnerMetrics,
+ ExperimentFeatures experimentFeatures) {
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
+ this.diffOperations = diffOperations;
this.autoMergerProvider = autoMergerProvider;
this.codeOwnerMetrics = codeOwnerMetrics;
+ this.experimentFeatures = experimentFeatures;
this.mergeStrategy = MergeUtil.getMergeStrategy(cfg);
}
/**
+ * Returns the changed files for the given revision.
+ *
+ * <p>By default the changed files are computed on access (see {@link #compute(Project.NameKey,
+ * ObjectId)}).
+ *
+ * <p>Only if enabled via the {@link CodeOwnersExperimentFeaturesConstants#USE_DIFF_CACHE}
+ * experiment feature flag the changed files are retrieved from the diff cache (see {@link
+ * #getFromDiffCache(Project.NameKey, ObjectId)}).
+ *
+ * @param project the project
+ * @param revision the revision for which the changed files should be computed
+ * @return the files that have been changed in the given revision, sorted alphabetically by path
+ */
+ public ImmutableList<ChangedFile> getOrCompute(Project.NameKey project, ObjectId revision)
+ throws IOException, PatchListNotAvailableException, DiffNotAvailableException {
+ if (experimentFeatures.isFeatureEnabled(CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)) {
+ if (isInitialCommit(project, revision)) {
+ // DiffOperations doesn't support getting the list of modified files for the initial commit.
+ return compute(project, revision);
+ }
+
+ return getFromDiffCache(project, revision);
+ }
+ return compute(project, revision);
+ }
+
+ /**
* Computes the files that have been changed in the given revision.
*
* <p>The diff is computed against the parent commit.
*
+ * <p>Rename detection is disabled.
+ *
* @param revisionResource the revision resource for which the changed files should be computed
* @return the files that have been changed in the given revision, sorted alphabetically by path
* @throws IOException thrown if the computation fails due to an I/O error
@@ -106,6 +160,8 @@
*
* <p>The diff is computed against the parent commit.
*
+ * <p>Rename detection is disabled.
+ *
* @param project the project
* @param revision the revision for which the changed files should be computed
* @return the files that have been changed in the given revision, sorted alphabetically by path
@@ -180,6 +236,8 @@
*
* <p>The computation also works if the commit doesn't have any parent.
*
+ * <p>Rename detection is disabled.
+ *
* @param repoConfig the repository configuration
* @param revWalk the rev walk
* @param commit the commit for which the changed files should be computed
@@ -215,4 +273,68 @@
}
}
}
+
+ /**
+ * Gets the changed files from the diff cache.
+ *
+ * <p>Doesn't support getting changed files for an initial revision. This is because the diff
+ * cache doesn't support getting changed files for commits that don't have any parent.
+ *
+ * <p>Rename detection is enabled.
+ *
+ * @throws IllegalStateException thrown if invoked for an initial revision
+ */
+ @VisibleForTesting
+ ImmutableList<ChangedFile> getFromDiffCache(Project.NameKey project, ObjectId revision)
+ throws IOException, DiffNotAvailableException {
+ requireNonNull(project, "project");
+ requireNonNull(revision, "revision");
+
+ checkState(!isInitialCommit(project, revision), "diff cache doesn't support initial commits");
+
+ MergeCommitStrategy mergeCommitStrategy =
+ codeOwnersPluginConfiguration.getProjectConfig(project).getMergeCommitStrategy();
+
+ try (Timer0.Context ctx = codeOwnerMetrics.getChangedFiles.start()) {
+ Map<String, FileDiffOutput> fileDiffOutputs;
+ if (mergeCommitStrategy.equals(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION)) {
+ // Use parentNum=null to do the comparison against the default base.
+ // For non-merge commits the default base is the only parent (aka parent 1, initial commits
+ // are not supported).
+ // For merge commits the default base is the auto-merge commit which should be used as base
+ // if the merge commit strategy is FILES_WITH_CONFLICT_RESOLUTION.
+ fileDiffOutputs =
+ diffOperations.listModifiedFilesAgainstParent(project, revision, /* parentNum=*/ null);
+ } else {
+ checkState(mergeCommitStrategy.equals(MergeCommitStrategy.ALL_CHANGED_FILES));
+ // Always use parent 1 to do the comparison.
+ // Non-merge commits should always be compared against against the first parent (initial
+ // commits are not supported).
+ // For merge commits also the first parent should be used if the merge commit strategy is
+ // ALL_CHANGED_FILES.
+ fileDiffOutputs = diffOperations.listModifiedFilesAgainstParent(project, revision, 1);
+ }
+
+ return toChangedFiles(filterOutMagicFilesAndSort(fileDiffOutputs)).collect(toImmutableList());
+ }
+ }
+
+ private boolean isInitialCommit(Project.NameKey project, ObjectId objectId) throws IOException {
+ try (Repository repo = repoManager.openRepository(project);
+ RevWalk revWalk = new RevWalk(repo)) {
+ return revWalk.parseCommit(objectId).getParentCount() == 0;
+ }
+ }
+
+ private Stream<Map.Entry<String, FileDiffOutput>> filterOutMagicFilesAndSort(
+ Map<String, FileDiffOutput> fileDiffOutputs) {
+ return fileDiffOutputs.entrySet().stream()
+ .filter(e -> !Patch.isMagic(e.getKey()))
+ .sorted(comparing(Map.Entry::getKey));
+ }
+
+ private Stream<ChangedFile> toChangedFiles(
+ Stream<Map.Entry<String, FileDiffOutput>> fileDiffOutputs) {
+ return fileDiffOutputs.map(Map.Entry::getValue).map(ChangedFile::create);
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 792087d..4317c77 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -44,6 +44,7 @@
import com.google.gerrit.server.git.PureRevertCache;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -158,7 +159,7 @@
ownedPaths = ownedPaths.limit(limit);
}
return ownedPaths.collect(toImmutableList());
- } catch (IOException | PatchListNotAvailableException e) {
+ } catch (IOException | PatchListNotAvailableException | DiffNotAvailableException e) {
throw new CodeOwnersInternalServerErrorException(
String.format(
"failed to compute owned paths of patch set %s for account %d",
@@ -174,7 +175,8 @@
* @return whether the given change has sufficient code owner approvals to be submittable
*/
public boolean isSubmittable(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
logger.atFine().log(
"checking if change %d in project %s is submittable",
@@ -209,17 +211,28 @@
* Gets the code owner statuses for all files/paths that were changed in the current revision of
* the given change as a set.
*
+ * @param start number of file statuses to skip
+ * @param limit the max number of file statuses that should be returned (0 = unlimited)
* @see #getFileStatuses(CodeOwnerConfigHierarchy, ChangeNotes)
*/
- public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(
+ ChangeNotes changeNotes, int start, int limit)
+ throws ResourceConflictException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) {
logger.atFine().log(
- "compute file statuses (project = %s, change = %d)",
- changeNotes.getProjectName(), changeNotes.getChangeId().get());
- return getFileStatuses(codeOwnerConfigHierarchyProvider.get(), changeNotes)
- .collect(toImmutableSet());
+ "compute file statuses (project = %s, change = %d, start = %d, limit = %d)",
+ changeNotes.getProjectName(), changeNotes.getChangeId().get(), start, limit);
+ Stream<FileCodeOwnerStatus> fileStatuses =
+ getFileStatuses(codeOwnerConfigHierarchyProvider.get(), changeNotes);
+ if (start > 0) {
+ fileStatuses = fileStatuses.skip(start);
+ }
+ if (limit > 0) {
+ fileStatuses = fileStatuses.limit(limit);
+ }
+ return fileStatuses.collect(toImmutableSet());
}
}
@@ -253,7 +266,8 @@
*/
private Stream<FileCodeOwnerStatus> getFileStatuses(
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy, ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.prepareFileStatusComputation.start()) {
logger.atFine().log(
@@ -326,7 +340,7 @@
FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
return changedFiles
- .compute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
+ .getOrCompute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
.stream()
.map(
changedFile ->
@@ -360,7 +374,8 @@
@VisibleForTesting
public Stream<FileCodeOwnerStatus> getFileStatusesForAccount(
ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId)
- throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
requireNonNull(patchSet, "patchSet");
requireNonNull(accountId, "accountId");
@@ -392,7 +407,7 @@
}
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
- return changedFiles.compute(changeNotes.getProjectName(), patchSet.commitId()).stream()
+ return changedFiles.getOrCompute(changeNotes.getProjectName(), patchSet.commitId()).stream()
.map(
changedFile ->
getFileStatus(
@@ -429,8 +444,8 @@
private Stream<FileCodeOwnerStatus> getAllPathsAsApproved(
ChangeNotes changeNotes, PatchSet patchSet)
- throws IOException, PatchListNotAvailableException {
- return changedFiles.compute(changeNotes.getProjectName(), patchSet.commitId()).stream()
+ throws IOException, PatchListNotAvailableException, DiffNotAvailableException {
+ return changedFiles.getOrCompute(changeNotes.getProjectName(), patchSet.commitId()).stream()
.map(
changedFile ->
FileCodeOwnerStatus.create(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index b0ac537..e69616d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -27,6 +27,7 @@
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.SubmitRule;
@@ -156,7 +157,8 @@
}
private SubmitRecord getSubmitRecord(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
return codeOwnerApprovalCheck.isSubmittable(changeNotes) ? ok() : notReady();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java
new file mode 100644
index 0000000..a0e82ad
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java
@@ -0,0 +1,38 @@
+// Copyright (C) 2021 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.plugins.codeowners.backend;
+
+/**
+ * Constants for {@link com.google.gerrit.server.experiments.ExperimentFeatures} in the code-owners
+ * plugin.
+ */
+public final class CodeOwnersExperimentFeaturesConstants {
+ /**
+ * Whether {@link com.google.gerrit.server.patch.DiffOperations}, and thus the diff cache, should
+ * be used to get changed files, instead of computing the changed files on our own.
+ *
+ * @see ChangedFiles#getOrCompute(com.google.gerrit.entities.Project.NameKey,
+ * org.eclipse.jgit.lib.ObjectId)
+ */
+ public static final String USE_DIFF_CACHE =
+ "GerritBackendRequestFeature__code_owners_use_diff_cache";
+
+ /**
+ * Private constructor to prevent instantiation of this class.
+ *
+ * <p>The class only contains static fields, hence the class never needs to be instantiated.
+ */
+ private CodeOwnersExperimentFeaturesConstants() {}
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java b/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java
index f66c31b..267e3c7 100644
--- a/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java
+++ b/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java
@@ -24,6 +24,7 @@
import com.google.gerrit.entities.Patch;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.patch.PatchListEntry;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import java.nio.file.Path;
import java.util.Optional;
import org.eclipse.jgit.diff.DiffEntry;
@@ -162,6 +163,26 @@
return Optional.ofNullable(path).map(newName -> JgitPath.of(newName).getAsAbsolutePath());
}
+ /**
+ * Creates a {@link ChangedFile} instance from a {@link FileDiffOutput}.
+ *
+ * @param fileDiffOutput the file diff output
+ */
+ public static ChangedFile create(FileDiffOutput fileDiffOutput) {
+ requireNonNull(fileDiffOutput, "fileDiffOutput");
+
+ return new AutoValue_ChangedFile(
+ convertPathFromFileDiffOutput(fileDiffOutput.newPath()),
+ convertPathFromFileDiffOutput(fileDiffOutput.oldPath()),
+ CHANGE_TYPE.get(fileDiffOutput.changeType()));
+ }
+
+ /** Converts the given string path to an absolute path. */
+ private static Optional<Path> convertPathFromFileDiffOutput(Optional<String> path) {
+ requireNonNull(path, "path");
+ return path.map(p -> JgitPath.of(p).getAsAbsolutePath());
+ }
+
public static ChangedFile create(
Optional<String> newPath, Optional<String> oldPath, ChangeType changeType) {
requireNonNull(changeType, "changeType");
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index 3bfcc54..89fac59 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -40,6 +40,7 @@
public final Timer0 computePatchSetApprovals;
public final Timer0 extendChangeMessageOnPostReview;
public final Timer0 getAutoMerge;
+ public final Timer0 getChangedFiles;
public final Timer0 prepareFileStatusComputation;
public final Timer0 prepareFileStatusComputationForAccount;
public final Timer0 resolveCodeOwnerConfig;
@@ -103,6 +104,9 @@
this.getAutoMerge =
createLatencyTimer(
"get_auto_merge", "Latency for getting the auto merge commit of a merge commit");
+ this.getChangedFiles =
+ createLatencyTimer(
+ "get_changed_files", "Latency for getting changed files from diff cache");
this.prepareFileStatusComputation =
createLatencyTimer(
"prepare_file_status_computation", "Latency for preparing the file status computation");
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java
index fdf7bd7..1952965 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java
@@ -14,7 +14,10 @@
package com.google.gerrit.plugins.codeowners.restapi;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestReadView;
@@ -22,11 +25,12 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerApprovalCheck;
import com.google.gerrit.plugins.codeowners.backend.FileCodeOwnerStatus;
import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
-import com.google.inject.Singleton;
import java.io.IOException;
+import org.kohsuke.args4j.Option;
/**
* REST endpoint that gets the code owner statuses for the files in a change.
@@ -48,10 +52,32 @@
* so that this approval can never happen)
* </ul>
*/
-@Singleton
public class GetCodeOwnerStatus implements RestReadView<ChangeResource> {
+ private static final int UNLIMITED = 0;
+
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
+ private int start;
+ private int limit;
+
+ @Option(
+ name = "--limit",
+ aliases = {"-n"},
+ metaVar = "CNT",
+ usage = "maximum number of file code owner statuses to return (by default 0 aka unlimited)")
+ public void setLimit(int limit) {
+ this.limit = limit;
+ }
+
+ @Option(
+ name = "--start",
+ aliases = {"-S"},
+ metaVar = "CNT",
+ usage = "number of owned paths to skip")
+ public void setStart(int start) {
+ this.start = start;
+ }
+
@Inject
public GetCodeOwnerStatus(CodeOwnerApprovalCheck codeOwnerApprovalCheck) {
this.codeOwnerApprovalCheck = codeOwnerApprovalCheck;
@@ -60,11 +86,29 @@
@Override
public Response<CodeOwnerStatusInfo> apply(ChangeResource changeResource)
throws RestApiException, IOException, PermissionBackendException,
- PatchListNotAvailableException {
+ PatchListNotAvailableException, DiffNotAvailableException {
+ validateStartAndLimit();
+
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(changeResource.getNotes());
- return Response.ok(
+ codeOwnerApprovalCheck.getFileStatusesAsSet(
+ changeResource.getNotes(), start, limit == UNLIMITED ? UNLIMITED : limit + 1);
+ CodeOwnerStatusInfo codeOwnerStatusInfo =
CodeOwnerStatusInfoJson.format(
- changeResource.getNotes().getCurrentPatchSet().id(), fileCodeOwnerStatuses));
+ changeResource.getNotes().getCurrentPatchSet().id(),
+ limit == UNLIMITED
+ ? fileCodeOwnerStatuses
+ : fileCodeOwnerStatuses.stream().limit(limit).collect(toImmutableSet()));
+ codeOwnerStatusInfo.more =
+ limit != UNLIMITED && fileCodeOwnerStatuses.size() > limit ? true : null;
+ return Response.ok(codeOwnerStatusInfo);
+ }
+
+ private void validateStartAndLimit() throws BadRequestException {
+ if (start < 0) {
+ throw new BadRequestException("start cannot be negative");
+ }
+ if (limit < 0) {
+ throw new BadRequestException("limit cannot be negative");
+ }
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerStatusInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerStatusInfoSubject.java
index 6bd2e07..a0b3452 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerStatusInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerStatusInfoSubject.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusInfoSubject.fileCodeOwnerStatusInfos;
import static com.google.gerrit.truth.ListSubject.elements;
+import com.google.common.truth.BooleanSubject;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.IntegerSubject;
import com.google.common.truth.Subject;
@@ -57,11 +58,15 @@
/** Returns a {@link ListSubject} for the file code owner statuses. */
public ListSubject<FileCodeOwnerStatusInfoSubject, FileCodeOwnerStatusInfo>
hasFileCodeOwnerStatusesThat() {
- return check("fileCodeOwnerStatuses")
+ return check("fileCodeOwnerStatuses()")
.about(elements())
.thatCustom(codeOwnerStatusInfo().fileCodeOwnerStatuses, fileCodeOwnerStatusInfos());
}
+ public BooleanSubject hasMoreThat() {
+ return check("more()").that(codeOwnerStatusInfo().more);
+ }
+
private CodeOwnerStatusInfo codeOwnerStatusInfo() {
isNotNull();
return codeOwnerStatusInfo;
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
index 79f235f..d8c51e6 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
@@ -14,17 +14,26 @@
package com.google.gerrit.plugins.codeowners.acceptance.api;
+import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerStatusInfoSubject.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusInfoSubject.isFileCodeOwnerStatus;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatusInfo;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnersExperimentFeaturesConstants;
import com.google.gerrit.plugins.codeowners.backend.FileCodeOwnerStatus;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
+import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.inject.Inject;
+import java.nio.file.Path;
+import java.nio.file.Paths;
import org.junit.Test;
/**
@@ -65,10 +74,376 @@
.hasFileCodeOwnerStatusesThat()
.comparingElementsUsing(isFileCodeOwnerStatus())
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
+ assertThat(codeOwnerStatus).hasMoreThat().isNull();
+ }
+
+ @Test
+ public void getStatusWithStart() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ String path1 = "/foo/bar/baz.md";
+ String path2 = "/foo/baz/bar.md";
+ String path3 = "/bar/foo.md";
+ String path4 = "/bar/baz.md";
+
+ PushOneCommit.Result r =
+ createChange(
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "file content",
+ JgitPath.of(path2).get(),
+ "file content",
+ JgitPath.of(path3).get(),
+ "file content",
+ JgitPath.of(path4).get(),
+ "file content"));
+ String changeId = r.getChangeId();
+
+ // Add a reviewer that is a code owner.
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ // Add a Code-Review+1 (= code owner approval) from a user that is not a code owner.
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId);
+
+ CodeOwnerStatusInfo codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withStart(0).get();
+ assertThat(codeOwnerStatus)
+ .hasPatchSetNumberThat()
+ .isEqualTo(r.getChange().currentPatchSet().id().get());
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path4, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path3, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.PENDING))
+ .inOrder();
+
+ codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withStart(1).get();
+ assertThat(codeOwnerStatus)
+ .hasPatchSetNumberThat()
+ .isEqualTo(r.getChange().currentPatchSet().id().get());
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path3, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.PENDING))
+ .inOrder();
+
+ codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withStart(2).get();
+ assertThat(codeOwnerStatus)
+ .hasPatchSetNumberThat()
+ .isEqualTo(r.getChange().currentPatchSet().id().get());
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.PENDING))
+ .inOrder();
+
+ codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withStart(3).get();
+ assertThat(codeOwnerStatus)
+ .hasPatchSetNumberThat()
+ .isEqualTo(r.getChange().currentPatchSet().id().get());
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.PENDING));
+
+ codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withStart(4).get();
+ assertThat(codeOwnerStatus)
+ .hasPatchSetNumberThat()
+ .isEqualTo(r.getChange().currentPatchSet().id().get());
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().isEmpty();
+ }
+
+ @Test
+ public void getStatusWithLimit() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ String path1 = "/foo/bar/baz.md";
+ String path2 = "/foo/baz/bar.md";
+ String path3 = "/bar/foo.md";
+ String path4 = "/bar/baz.md";
+
+ PushOneCommit.Result r =
+ createChange(
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "file content",
+ JgitPath.of(path2).get(),
+ "file content",
+ JgitPath.of(path3).get(),
+ "file content",
+ JgitPath.of(path4).get(),
+ "file content"));
+ String changeId = r.getChangeId();
+
+ // Add a reviewer that is a code owner.
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ // Add a Code-Review+1 (= code owner approval) from a user that is not a code owner.
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId);
+
+ CodeOwnerStatusInfo codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withLimit(1).get();
+ assertThat(codeOwnerStatus)
+ .hasPatchSetNumberThat()
+ .isEqualTo(r.getChange().currentPatchSet().id().get());
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path4, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ assertThat(codeOwnerStatus).hasMoreThat().isTrue();
+
+ codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withLimit(2).get();
+ assertThat(codeOwnerStatus)
+ .hasPatchSetNumberThat()
+ .isEqualTo(r.getChange().currentPatchSet().id().get());
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path4, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path3, CodeOwnerStatus.INSUFFICIENT_REVIEWERS))
+ .inOrder();
+ assertThat(codeOwnerStatus).hasMoreThat().isTrue();
+
+ codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withLimit(3).get();
+ assertThat(codeOwnerStatus)
+ .hasPatchSetNumberThat()
+ .isEqualTo(r.getChange().currentPatchSet().id().get());
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path4, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path3, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.PENDING))
+ .inOrder();
+ assertThat(codeOwnerStatus).hasMoreThat().isTrue();
+
+ codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withLimit(4).get();
+ assertThat(codeOwnerStatus)
+ .hasPatchSetNumberThat()
+ .isEqualTo(r.getChange().currentPatchSet().id().get());
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path4, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path3, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.PENDING))
+ .inOrder();
+ assertThat(codeOwnerStatus).hasMoreThat().isNull();
+ }
+
+ @Test
+ public void getStatusWithLimitForRename() throws Exception {
+ testGetStatusWithLimitForRenamedFile(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusWithLimitForRename_useDiffCache() throws Exception {
+ testGetStatusWithLimitForRenamedFile(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusWithLimitForRenamedFile(boolean useDiffCache) throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ Path oldPath = Paths.get("/foo/old.bar");
+ Path newPath = Paths.get("/bar/new.bar");
+ String changeId = createChangeWithFileRename(oldPath, newPath);
+
+ // Add a reviewer that is a code owner of the old path.
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ CodeOwnerStatusInfo codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withLimit(1).get();
+ if (useDiffCache) {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.PENDING,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ assertThat(codeOwnerStatus).hasMoreThat().isNull();
+ } else {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ assertThat(codeOwnerStatus).hasMoreThat().isTrue();
+
+ codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withLimit(2).get();
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING));
+ assertThat(codeOwnerStatus).hasMoreThat().isNull();
+ }
+ }
+
+ @Test
+ public void getStatusWithStartAndLimit() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ String path1 = "/foo/bar/baz.md";
+ String path2 = "/foo/baz/bar.md";
+ String path3 = "/bar/foo.md";
+ String path4 = "/bar/baz.md";
+
+ PushOneCommit.Result r =
+ createChange(
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "file content",
+ JgitPath.of(path2).get(),
+ "file content",
+ JgitPath.of(path3).get(),
+ "file content",
+ JgitPath.of(path4).get(),
+ "file content"));
+ String changeId = r.getChangeId();
+
+ // Add a reviewer that is a code owner.
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ // Add a Code-Review+1 (= code owner approval) from a user that is not a code owner.
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId);
+
+ CodeOwnerStatusInfo codeOwnerStatus =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .getCodeOwnerStatus()
+ .withStart(1)
+ .withLimit(2)
+ .get();
+ assertThat(codeOwnerStatus)
+ .hasPatchSetNumberThat()
+ .isEqualTo(r.getChange().currentPatchSet().id().get());
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path3, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.PENDING))
+ .inOrder();
+ assertThat(codeOwnerStatus).hasMoreThat().isTrue();
+ }
+
+ @Test
+ public void startCannotBeNegative() throws Exception {
+ String changeId = createChange().getChangeId();
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .getCodeOwnerStatus()
+ .withStart(-1)
+ .get());
+ assertThat(exception).hasMessageThat().isEqualTo("start cannot be negative");
+ }
+
+ @Test
+ public void limitCannotBeNegative() throws Exception {
+ String changeId = createChange().getChangeId();
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .getCodeOwnerStatus()
+ .withLimit(-1)
+ .get());
+ assertThat(exception).hasMessageThat().isEqualTo("limit cannot be negative");
+ }
+
+ @Test
+ public void getStatusWithoutLimit() throws Exception {
+ String changeId = createChange().getChangeId();
+ CodeOwnerStatusInfo codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withLimit(0).get();
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().isNotEmpty();
+ assertThat(codeOwnerStatus).hasMoreThat().isNull();
}
@Test
public void getStatusForRenamedFile() throws Exception {
+ testGetStatusForRenamedFile(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForRenamedFile_useDiffCache() throws Exception {
+ testGetStatusForRenamedFile(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForRenamedFile(boolean useDiffCache) throws Exception {
TestAccount user2 = accountCreator.user2();
setAsCodeOwners("/foo/bar/", user);
@@ -80,34 +455,67 @@
CodeOwnerStatusInfo codeOwnerStatus =
changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ if (useDiffCache) {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
// Add a reviewer that is a code owner of the old path.
gApi.changes().id(changeId).addReviewer(user.email());
codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ if (useDiffCache) {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.PENDING,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
// Add a reviewer that is a code owner of the new path.
gApi.changes().id(changeId).addReviewer(user2.email());
codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.PENDING));
+ if (useDiffCache) {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath, CodeOwnerStatus.PENDING, newPath, CodeOwnerStatus.PENDING));
+ } else {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.PENDING));
+ }
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnerStatusRestIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnerStatusRestIT.java
index a4e6437..8f4959d 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnerStatusRestIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnerStatusRestIT.java
@@ -88,6 +88,28 @@
filePath, project.get()));
}
+ @Test
+ public void cannotGetStatusWithInvalidStart() throws Exception {
+ String changeId = createChange().getChangeId();
+ RestResponse r =
+ adminRestSession.get(
+ String.format(
+ "/changes/%s/code_owners.status?start=invalid", IdString.fromDecoded(changeId)));
+ r.assertBadRequest();
+ assertThat(r.getEntityContent()).contains("\"invalid\" is not a valid value for \"--start\"");
+ }
+
+ @Test
+ public void cannotGetStatusWithInvalidLimit() throws Exception {
+ String changeId = createChange().getChangeId();
+ RestResponse r =
+ adminRestSession.get(
+ String.format(
+ "/changes/%s/code_owners.status?limit=invalid", IdString.fromDecoded(changeId)));
+ r.assertBadRequest();
+ assertThat(r.getEntityContent()).contains("\"invalid\" is not a valid value for \"--limit\"");
+ }
+
private CodeOwnerConfig.Key createCodeOwnerConfigKey(String folderPath) {
return CodeOwnerConfig.Key.create(project, "master", folderPath);
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
index a1ac734..2c84634 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
@@ -24,7 +24,9 @@
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.common.ChangedFile;
import com.google.gerrit.server.patch.PatchListEntry;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import java.nio.file.Paths;
+import java.util.Optional;
import org.eclipse.jgit.diff.DiffEntry;
import org.eclipse.jgit.diff.DiffEntry.ChangeType;
import org.junit.Rule;
@@ -40,6 +42,7 @@
@Mock private DiffEntry diffEntry;
@Mock private PatchListEntry patchListEntry;
+ @Mock private FileDiffOutput fileDiffOutput;
@Test
public void getNewPath_diffEntry() throws Exception {
@@ -58,6 +61,15 @@
}
@Test
+ public void getNewPath_fileDiffOutput() throws Exception {
+ String newPath = "foo/bar/baz.txt";
+ setupFileDiffOutput(newPath, /* oldPath= */ null, Patch.ChangeType.ADDED);
+ assertThat(ChangedFile.create(fileDiffOutput).newPath())
+ .value()
+ .isEqualTo(Paths.get("/" + newPath));
+ }
+
+ @Test
public void getNewPathWhenNewPathIsNotSet_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
assertThat(ChangedFile.create(diffEntry).newPath()).isEmpty();
@@ -70,6 +82,12 @@
}
@Test
+ public void getNewPathWhenNewPathIsNotSet_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
+ assertThat(ChangedFile.create(fileDiffOutput).newPath()).isEmpty();
+ }
+
+ @Test
public void hasNewPath_diffEntry() throws Exception {
String newPath = "foo/bar/baz.txt";
setupDiffEntry(newPath, /* oldPath= */ null, ChangeType.ADD);
@@ -90,6 +108,16 @@
}
@Test
+ public void hasNewPath_fileDiffOutput() throws Exception {
+ String newPath = "foo/bar/baz.txt";
+ setupFileDiffOutput(newPath, /* oldPath= */ null, Patch.ChangeType.ADDED);
+
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.hasNewPath(Paths.get("/" + newPath))).isTrue();
+ assertThat(changedFile.hasNewPath(Paths.get("/otherPath"))).isFalse();
+ }
+
+ @Test
public void cannotCheckHasNewPathWithNull_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
@@ -112,6 +140,17 @@
}
@Test
+ public void cannotCheckHasNewPathWithNull_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
+
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> ChangedFile.create(fileDiffOutput).hasNewPath(/* absolutePath= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("absolutePath");
+ }
+
+ @Test
public void cannotCheckHasNewPathWithRelativePath_diffEntry() throws Exception {
String relativePath = "foo/bar/baz.txt";
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
@@ -138,6 +177,19 @@
}
@Test
+ public void cannotCheckHasNewPathWithRelativePath_fileDiffOutput() throws Exception {
+ String relativePath = "foo/bar/baz.txt";
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
+ IllegalStateException exception =
+ assertThrows(
+ IllegalStateException.class,
+ () -> ChangedFile.create(fileDiffOutput).hasNewPath(Paths.get(relativePath)));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(String.format("path %s must be absolute", relativePath));
+ }
+
+ @Test
public void getOldPath_diffEntry() throws Exception {
String oldPath = "foo/bar/baz.txt";
setupDiffEntry(/* newPath= */ null, oldPath, ChangeType.DELETE);
@@ -154,6 +206,15 @@
}
@Test
+ public void getOldPath_fileDiffOutput() throws Exception {
+ String oldPath = "foo/bar/baz.txt";
+ setupFileDiffOutput(/* newPath= */ null, oldPath, Patch.ChangeType.DELETED);
+ assertThat(ChangedFile.create(fileDiffOutput).oldPath())
+ .value()
+ .isEqualTo(Paths.get("/" + oldPath));
+ }
+
+ @Test
public void getOldPathWhenOldPathIsNotSet_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
when(diffEntry.getOldPath()).thenReturn(DiffEntry.DEV_NULL);
@@ -167,6 +228,12 @@
}
@Test
+ public void getOldPathWhenOldPathIsNotSet_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
+ assertThat(ChangedFile.create(fileDiffOutput).oldPath()).isEmpty();
+ }
+
+ @Test
public void hasOldPath_diffEntry() throws Exception {
String oldPath = "foo/bar/baz.txt";
setupDiffEntry(/* newPath= */ null, oldPath, ChangeType.DELETE);
@@ -187,6 +254,16 @@
}
@Test
+ public void hasOldPath_fileDiffOutput() throws Exception {
+ String oldPath = "foo/bar/baz.txt";
+ setupFileDiffOutput(/* newPath= */ null, oldPath, Patch.ChangeType.DELETED);
+
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.hasOldPath(Paths.get("/" + oldPath))).isTrue();
+ assertThat(changedFile.hasOldPath(Paths.get("/otherPath"))).isFalse();
+ }
+
+ @Test
public void cannotCheckHasOldPathWithNull_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
NullPointerException npe =
@@ -207,6 +284,16 @@
}
@Test
+ public void cannotCheckHasOldPathWithNull_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> ChangedFile.create(fileDiffOutput).hasOldPath(/* absolutePath= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("absolutePath");
+ }
+
+ @Test
public void cannotCheckHasOldPathWithRelativePath_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
String relativePath = "foo/bar/baz.txt";
@@ -233,6 +320,19 @@
}
@Test
+ public void cannotCheckHasOldPathWithRelativePath_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
+ String relativePath = "foo/bar/baz.txt";
+ IllegalStateException exception =
+ assertThrows(
+ IllegalStateException.class,
+ () -> ChangedFile.create(fileDiffOutput).hasOldPath(Paths.get(relativePath)));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(String.format("path %s must be absolute", relativePath));
+ }
+
+ @Test
public void isRename_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.RENAME);
ChangedFile changedFile = ChangedFile.create(diffEntry);
@@ -249,6 +349,14 @@
}
@Test
+ public void isRename_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.RENAMED);
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.isRename()).isTrue();
+ assertThat(changedFile.isDeletion()).isFalse();
+ }
+
+ @Test
public void isDeletion_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
ChangedFile changedFile = ChangedFile.create(diffEntry);
@@ -265,6 +373,14 @@
}
@Test
+ public void isDeletion_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.isRename()).isFalse();
+ assertThat(changedFile.isDeletion()).isTrue();
+ }
+
+ @Test
public void isAddition_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
ChangedFile changedFile = ChangedFile.create(diffEntry);
@@ -281,6 +397,14 @@
}
@Test
+ public void isAddition_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.isRename()).isFalse();
+ assertThat(changedFile.isDeletion()).isFalse();
+ }
+
+ @Test
public void isModify_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.MODIFY);
ChangedFile changedFile = ChangedFile.create(diffEntry);
@@ -297,6 +421,14 @@
}
@Test
+ public void isModify_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.MODIFIED);
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.isRename()).isFalse();
+ assertThat(changedFile.isDeletion()).isFalse();
+ }
+
+ @Test
public void isCopy_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.COPY);
ChangedFile changedFile = ChangedFile.create(diffEntry);
@@ -312,6 +444,14 @@
assertThat(changedFile.isDeletion()).isFalse();
}
+ @Test
+ public void isCopy_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.COPIED);
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.isRename()).isFalse();
+ assertThat(changedFile.isDeletion()).isFalse();
+ }
+
private void setupDiffEntry(
@Nullable String newPath, @Nullable String oldPath, ChangeType changeType) {
when(diffEntry.getNewPath()).thenReturn(newPath != null ? newPath : DiffEntry.DEV_NULL);
@@ -331,4 +471,11 @@
when(patchListEntry.getChangeType()).thenReturn(changeType);
}
}
+
+ private void setupFileDiffOutput(
+ @Nullable String newPath, @Nullable String oldPath, Patch.ChangeType changeType) {
+ when(fileDiffOutput.newPath()).thenReturn(Optional.ofNullable(newPath));
+ when(fileDiffOutput.oldPath()).thenReturn(Optional.ofNullable(oldPath));
+ when(fileDiffOutput.changeType()).thenReturn(changeType);
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
index 8e70676..4534df2 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -47,6 +47,7 @@
import com.google.inject.Inject;
import java.nio.file.Paths;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@@ -335,7 +336,7 @@
}
@Test
- public void sortedByPath() throws Exception {
+ public void computeReturnsChangedFilesSortedByPath() throws Exception {
String file1 = "foo/bar.baz";
String file2 = "foo/baz.bar";
String file3 = "bar/foo.baz";
@@ -367,19 +368,23 @@
@Test
@GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
- public void sortedByPath_mergeCommitAgainstFirstParent() throws Exception {
- testSortedByPathForMerge(MergeCommitStrategy.ALL_CHANGED_FILES);
+ public void computeReturnsChangedFilesSortedByPath_mergeCommitAgainstFirstParent()
+ throws Exception {
+ testComputeReturnsChangedFilesSortedByPathForMerge(MergeCommitStrategy.ALL_CHANGED_FILES);
}
@Test
@GerritConfig(
name = "plugin.code-owners.mergeCommitStrategy",
value = "FILES_WITH_CONFLICT_RESOLUTION")
- public void sortedByPath_mergeCommitAgainstAutoMerge() throws Exception {
- testSortedByPathForMerge(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
+ public void computeReturnsChangedFilesSortedByPath_mergeCommitAgainstAutoMerge()
+ throws Exception {
+ testComputeReturnsChangedFilesSortedByPathForMerge(
+ MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
}
- private void testSortedByPathForMerge(MergeCommitStrategy mergeCommitStrategy) throws Exception {
+ private void testComputeReturnsChangedFilesSortedByPathForMerge(
+ MergeCommitStrategy mergeCommitStrategy) throws Exception {
setAsRootCodeOwners(admin);
String file1 = "foo/bar.baz";
@@ -481,6 +486,416 @@
}
}
+ @Test
+ public void cannotGetFromDiffCacheForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> changedFiles.getFromDiffCache(/* project= */ null, ObjectId.zeroId()));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
+ }
+
+ @Test
+ public void cannotGetFromDiffCacheForNullRevision() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> changedFiles.getFromDiffCache(project, /* revision= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("revision");
+ }
+
+ @Test
+ public void getFromDiffCacheForChangeThatAddedAFile() throws Exception {
+ String path = "/foo/bar/baz.txt";
+ RevCommit commit =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getCommit();
+
+ ImmutableList<ChangedFile> changedFilesSet = changedFiles.getFromDiffCache(project, commit);
+ assertThat(changedFilesSet).hasSize(1);
+ ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
+ assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
+ assertThat(changedFile).hasOldPath().isEmpty();
+ assertThat(changedFile).isNoRename();
+ assertThat(changedFile).isNoDeletion();
+ }
+
+ @Test
+ public void getFromDiffCacheForChangeThatModifiedAFile() throws Exception {
+ String path = "/foo/bar/baz.txt";
+ createChange("Test Change", JgitPath.of(path).get(), "file content").getChangeId();
+
+ RevCommit commit =
+ createChange("Change Modifying A File", JgitPath.of(path).get(), "new file content")
+ .getCommit();
+
+ ImmutableList<ChangedFile> changedFilesSet = changedFiles.getFromDiffCache(project, commit);
+ assertThat(changedFilesSet).hasSize(1);
+ ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
+ assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
+ assertThat(changedFile).hasOldPath().value().isEqualTo(Paths.get(path));
+ assertThat(changedFile).isNoRename();
+ assertThat(changedFile).isNoDeletion();
+ }
+
+ @Test
+ public void getFromDiffCacheForChangeThatDeletedAFile() throws Exception {
+ String path = "/foo/bar/baz.txt";
+ String changeId = createChangeWithFileDeletion(path);
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(
+ project, getRevisionResource(changeId).getPatchSet().commitId());
+ assertThat(changedFilesSet).hasSize(1);
+ ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
+ assertThat(changedFile).hasNewPath().isEmpty();
+ assertThat(changedFile).hasOldPath().value().isEqualTo(Paths.get(path));
+ assertThat(changedFile).isNoRename();
+ assertThat(changedFile).isDeletion();
+ }
+
+ @Test
+ public void getFromDiffCacheForChangeThatRenamedAFile() throws Exception {
+ String oldPath = "/foo/bar/old.txt";
+ String newPath = "/foo/bar/new.txt";
+ String changeId = createChangeWithFileRename(oldPath, newPath);
+
+ gApi.changes().id(changeId).current().files();
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(
+ project, getRevisionResource(changeId).getPatchSet().commitId());
+ ChangedFileSubject changedFile = assertThatCollection(changedFilesSet).onlyElement();
+ changedFile.hasNewPath().value().isEqualTo(Paths.get(newPath));
+ changedFile.hasOldPath().value().isEqualTo(Paths.get(oldPath));
+ changedFile.isRename();
+ changedFile.isNoDeletion();
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void getFromDiffCacheForInitialChangeThatAddedAFile() throws Exception {
+ String path = "/foo/bar/baz.txt";
+ RevCommit commit =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getCommit();
+ assertThat(commit.getParents()).isEmpty();
+
+ IllegalStateException exception =
+ assertThrows(
+ IllegalStateException.class, () -> changedFiles.getFromDiffCache(project, commit));
+ assertThat(exception).hasMessageThat().isEqualTo("diff cache doesn't support initial commits");
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
+ public void getFromFileDiffCacheForMergeChange_allChangedFiles() throws Exception {
+ testGetFromFileDiffCacheForMergeChange(MergeCommitStrategy.ALL_CHANGED_FILES);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.mergeCommitStrategy",
+ value = "FILES_WITH_CONFLICT_RESOLUTION")
+ public void getFromFileDiffCacheForMergeChange_filesWithConflictResolution() throws Exception {
+ testGetFromFileDiffCacheForMergeChange(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
+ }
+
+ private void testGetFromFileDiffCacheForMergeChange(MergeCommitStrategy mergeCommitStrategy)
+ throws Exception {
+ setAsRootCodeOwners(admin);
+
+ String file1 = "foo/a.txt";
+ String file2 = "bar/b.txt";
+
+ // Create a base change.
+ Change.Id baseChange =
+ changeOperations.newChange().branch("master").file(file1).content("base content").create();
+ approveAndSubmit(baseChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1.
+ Change.Id changeInMaster =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .file(file1)
+ .content("master content")
+ .create();
+ approveAndSubmit(changeInMaster);
+
+ // Create a change in the other branch and that touches file1 and creates file2.
+ Change.Id changeInOtherBranch =
+ changeOperations
+ .newChange()
+ .branch(branchName)
+ .file(file1)
+ .content("other content")
+ .file(file2)
+ .content("content")
+ .create();
+ approveAndSubmit(changeInOtherBranch);
+
+ // Create a merge change with a conflict resolution for file1 and file2 with the same content as
+ // in the other branch (no conflict on file2).
+ Change.Id mergeChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .mergeOfButBaseOnFirst()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file1)
+ .content("merged content")
+ .file(file2)
+ .content("content")
+ .create();
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(
+ project,
+ getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId());
+
+ if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
+ assertThat(changedFilesSet).comparingElementsUsing(hasPath()).containsExactly(file1, file2);
+ } else if (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) {
+ assertThat(changedFilesSet).comparingElementsUsing(hasPath()).containsExactly(file1);
+ } else {
+ fail("expected merge commit strategy: " + mergeCommitStrategy);
+ }
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
+ public void
+ getFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution_allChangedFiles()
+ throws Exception {
+ testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.mergeCommitStrategy",
+ value = "FILES_WITH_CONFLICT_RESOLUTION")
+ public void
+ getFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution_filesWithConflictResolution()
+ throws Exception {
+ testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution();
+ }
+
+ private void testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution()
+ throws Exception {
+ setAsRootCodeOwners(admin);
+
+ String file = "foo/a.txt";
+
+ // Create a base change.
+ Change.Id baseChange =
+ changeOperations.newChange().branch("master").file(file).content("base content").create();
+ approveAndSubmit(baseChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1.
+ Change.Id changeInMaster =
+ changeOperations.newChange().branch("master").file(file).content("master content").create();
+ approveAndSubmit(changeInMaster);
+
+ // Create a change in the other branch and that deleted file1.
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change Deleting A File", file, "");
+ Result r = push.rm("refs/for/master");
+ r.assertOkStatus();
+ approveAndSubmit(r.getChange().getId());
+
+ // Create a merge change with resolving the conflict on file between the edit in master and the
+ // deletion in the other branch by deleting the file.
+ Change.Id mergeChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .mergeOf()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file)
+ .delete()
+ .create();
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(
+ project,
+ getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId());
+ ImmutableSet<String> oldPaths =
+ changedFilesSet.stream()
+ .map(changedFile -> JgitPath.of(changedFile.oldPath().get()).get())
+ .collect(toImmutableSet());
+ assertThat(oldPaths).containsExactly(file);
+ }
+
+ @Test
+ public void getFromDiffCacheReturnsChangedFilesSortedByPath() throws Exception {
+ String file1 = "foo/bar.baz";
+ String file2 = "foo/baz.bar";
+ String file3 = "bar/foo.baz";
+ String file4 = "bar/baz.foo";
+ String file5 = "baz/foo.bar";
+ RevCommit commit =
+ createChange(
+ "Test Change",
+ ImmutableMap.of(
+ file1,
+ "file content",
+ file2,
+ "file content",
+ file3,
+ "file content",
+ file4,
+ "file content",
+ file5,
+ "file content"))
+ .getCommit();
+
+ ImmutableList<ChangedFile> changedFilesSet = changedFiles.getFromDiffCache(project, commit);
+ assertThat(changedFilesSet)
+ .comparingElementsUsing(hasPath())
+ .containsExactly(file4, file3, file5, file1, file2)
+ .inOrder();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
+ public void getFromDiffCacheReturnsChangedFilesSortedByPath_mergeCommitAgainstFirstParent()
+ throws Exception {
+ testGetFromDiffCacheReturnsChangedFilesSortedByPathForMerge(
+ MergeCommitStrategy.ALL_CHANGED_FILES);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.mergeCommitStrategy",
+ value = "FILES_WITH_CONFLICT_RESOLUTION")
+ public void getFromDiffCacheReturnsChangedFilesSortedByPath_mergeCommitAgainstAutoMerge()
+ throws Exception {
+ testGetFromDiffCacheReturnsChangedFilesSortedByPathForMerge(
+ MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
+ }
+
+ private void testGetFromDiffCacheReturnsChangedFilesSortedByPathForMerge(
+ MergeCommitStrategy mergeCommitStrategy) throws Exception {
+ setAsRootCodeOwners(admin);
+
+ String file1 = "foo/bar.baz";
+ String file2 = "foo/baz.bar";
+ String file3 = "bar/foo.baz";
+ String file4 = "bar/baz.foo";
+ String file5 = "baz/foo.bar";
+
+ // Create a base change.
+ Change.Id baseChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .file(file1)
+ .content("base content")
+ .file(file3)
+ .content("base content")
+ .file(file5)
+ .content("base content")
+ .create();
+ approveAndSubmit(baseChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1, file3 and file5
+ Change.Id changeInMaster =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .file(file1)
+ .content("master content")
+ .file(file3)
+ .content("master content")
+ .file(file5)
+ .content("master content")
+ .create();
+ approveAndSubmit(changeInMaster);
+
+ // Create a change in the other branch and that touches file1, file3, file5 and creates file2,
+ // file4.
+ Change.Id changeInOtherBranch =
+ changeOperations
+ .newChange()
+ .branch(branchName)
+ .file(file1)
+ .content("other content")
+ .file(file2)
+ .content("content")
+ .file(file3)
+ .content("other content")
+ .file(file4)
+ .content("content")
+ .file(file5)
+ .content("other content")
+ .create();
+ approveAndSubmit(changeInOtherBranch);
+
+ // Create a merge change with a conflict resolution for file1 and file2 with the same content as
+ // in the other branch (no conflict on file2).
+ Change.Id mergeChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .mergeOfButBaseOnFirst()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file1)
+ .content("merged content")
+ .file(file2)
+ .content("content")
+ .file(file3)
+ .content("merged content")
+ .file(file4)
+ .content("content")
+ .file(file5)
+ .content("merged content")
+ .create();
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(
+ project,
+ getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId());
+
+ if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
+ assertThat(changedFilesSet)
+ .comparingElementsUsing(hasPath())
+ .containsExactly(file4, file3, file5, file1, file2)
+ .inOrder();
+ } else if (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) {
+ assertThat(changedFilesSet)
+ .comparingElementsUsing(hasPath())
+ .containsExactly(file3, file5, file1);
+ } else {
+ fail("expected merge commit strategy: " + mergeCommitStrategy);
+ }
+ }
+
private void approveAndSubmit(Change.Id changeId) throws Exception {
approve(Integer.toString(changeId.get()));
gApi.changes().id(changeId.get()).current().submit();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 596eead..566c5cd 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -79,7 +79,9 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> codeOwnerApprovalCheck.getFileStatusesAsSet(/* changeNotes= */ null));
+ () ->
+ codeOwnerApprovalCheck.getFileStatusesAsSet(
+ /* changeNotes= */ null, /* start= */ 0, /* limit= */ 0));
assertThat(npe).hasMessageThat().isEqualTo("changeNotes");
}
@@ -98,8 +100,7 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -122,8 +123,7 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.modification(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -143,8 +143,7 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.deletion(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -152,6 +151,19 @@
@Test
public void getStatusForFileRename_insufficientReviewers() throws Exception {
+ testGetStatusForFileRename_insufficientReviewers(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_insufficientReviewers_useDiffCache() throws Exception {
+ testGetStatusForFileRename_insufficientReviewers(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForFileRename_insufficientReviewers(boolean useDiffCache)
+ throws Exception {
TestAccount user2 = accountCreator.user2();
Path oldPath = Paths.get("/foo/old.bar");
@@ -165,12 +177,21 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
}
@Test
@@ -190,8 +211,7 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
}
@@ -215,8 +235,7 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.modification(path, CodeOwnerStatus.PENDING));
}
@@ -237,14 +256,25 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.deletion(path, CodeOwnerStatus.PENDING));
}
@Test
public void getStatusForFileRename_pendingOldPath() throws Exception {
+ testGetStatusForFileRename_pendingOldPath(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_pendingOldPath_useDiffCache() throws Exception {
+ testGetStatusForFileRename_pendingOldPath(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForFileRename_pendingOldPath(boolean useDiffCache) throws Exception {
TestAccount user2 = accountCreator.user2();
setAsCodeOwners("/foo/bar/", user);
@@ -260,16 +290,37 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.PENDING,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
}
@Test
public void getStatusForFileRename_pendingNewPath() throws Exception {
+ testGetStatusForFileRename_pendingNewPath(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_pendingNewPath_useDiffCache() throws Exception {
+ testGetStatusForFileRename_pendingNewPath(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForFileRename_pendingNewPath(boolean useDiffCache) throws Exception {
TestAccount user2 = accountCreator.user2();
setAsCodeOwners("/foo/baz/", user);
@@ -285,12 +336,21 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.PENDING));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.PENDING));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.PENDING));
+ }
}
@Test
@@ -305,8 +365,7 @@
requestScopeOperations.setApiUser(user.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -325,8 +384,7 @@
requestScopeOperations.setApiUser(user.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.modification(path, CodeOwnerStatus.APPROVED));
}
@@ -342,14 +400,25 @@
requestScopeOperations.setApiUser(user.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.deletion(path, CodeOwnerStatus.APPROVED));
}
@Test
public void getStatusForFileRename_approvedOldPath() throws Exception {
+ testGetStatusForFileRename_approvedOldPath(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_approvedOldPath_useDiffCache() throws Exception {
+ testGetStatusForFileRename_approvedOldPath(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForFileRename_approvedOldPath(boolean useDiffCache) throws Exception {
setAsCodeOwners("/foo/bar/", user);
Path oldPath = Paths.get("/foo/bar/abc.txt");
@@ -361,16 +430,37 @@
requestScopeOperations.setApiUser(user.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.APPROVED),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.APPROVED,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.APPROVED),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
}
@Test
public void getStatusForFileRename_approvedNewPath() throws Exception {
+ testGetStatusForFileRename_approvedNewPath(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_approvedNewPath_useDiffCache() throws Exception {
+ testGetStatusForFileRename_approvedNewPath(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForFileRename_approvedNewPath(boolean useDiffCache) throws Exception {
setAsCodeOwners("/foo/baz/", user);
Path oldPath = Paths.get("/foo/bar/abc.txt");
@@ -382,12 +472,21 @@
requestScopeOperations.setApiUser(user.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.APPROVED));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.APPROVED));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.APPROVED));
+ }
}
@Test
@@ -412,8 +511,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
amendChange(user, changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
@@ -448,8 +546,7 @@
.getChangeId();
amendChange(user, changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.modification(
@@ -480,8 +577,7 @@
String changeId = createChangeWithFileDeletion(path);
amendChange(user, changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.deletion(
@@ -495,7 +591,17 @@
public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnOldPath()
throws Exception {
testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ false);
+ /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnOldPath_useDiffCache()
+ throws Exception {
+ testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
+ /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ true);
}
@Test
@@ -503,11 +609,22 @@
public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnOldPath()
throws Exception {
testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ true);
+ /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnOldPath_useDiffCache()
+ throws Exception {
+ testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
+ /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ true);
}
private void testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- boolean implicitApprovalsEnabled) throws Exception {
+ boolean implicitApprovalsEnabled, boolean useDiffCache) throws Exception {
setAsCodeOwners("/foo/bar/", user);
Path oldPath = Paths.get("/foo/bar/abc.txt");
@@ -515,23 +632,44 @@
String changeId = createChangeWithFileRename(oldPath, newPath);
amendChange(user, changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(
- oldPath,
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(
+ oldPath,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
}
@Test
public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnNewPath()
throws Exception {
testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ false);
+ /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnNewPath_useDiffCache()
+ throws Exception {
+ testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
+ /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ true);
}
@Test
@@ -539,11 +677,22 @@
public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnNewPath()
throws Exception {
testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ true);
+ /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnNewPath_useDiffCache()
+ throws Exception {
+ testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
+ /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ true);
}
private void testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- boolean implicitApprovalsEnabled) throws Exception {
+ boolean implicitApprovalsEnabled, boolean useDiffCache) throws Exception {
setAsCodeOwners("/foo/baz/", user);
Path oldPath = Paths.get("/foo/bar/abc.txt");
@@ -551,16 +700,27 @@
String changeId = createChangeWithFileRename(oldPath, newPath);
amendChange(user, changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(
- newPath,
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(
+ newPath,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
}
@Test
@@ -573,8 +733,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
amendChange(user, changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -594,8 +753,7 @@
amendChange(user, changeId);
amendChange(user2, changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -620,8 +778,7 @@
// Verify that the file is not approved yet (the change owner is a code owner, but
// implicit approvals are disabled).
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -631,7 +788,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -663,8 +820,7 @@
String changeId =
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
@@ -692,8 +848,7 @@
// Verify that the status of the file is INSUFFICIENT_REVIEWERS (since there is no implicit
// approval by default).
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -702,7 +857,7 @@
gApi.changes().id(changeId).addReviewer(user.email());
// Check that the status of the file is PENDING now.
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
}
@@ -721,8 +876,7 @@
.getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -738,7 +892,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -766,8 +920,7 @@
createChange(bot, "Change Adding A File", JgitPath.of(path).get(), "file content")
.getChangeId();
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
@@ -791,8 +944,7 @@
.getChangeId();
// Verify that the status of the file is INSUFFICIENT_REVIEWERS.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -802,7 +954,7 @@
// Check that the status of the file is PENDING now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
@@ -817,7 +969,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -833,8 +985,7 @@
// Verify that the file is not approved yet (the change owner is a global code owner, but
// implicit approvals are disabled).
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -844,7 +995,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -871,8 +1022,7 @@
String changeId =
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
@@ -892,8 +1042,7 @@
// Verify that the status of the file is INSUFFICIENT_REVIEWERS (since there is no implicit
// approval by default).
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -902,7 +1051,7 @@
gApi.changes().id(changeId).addReviewer(user.email());
// Check that the status of the file is PENDING now.
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
}
@@ -929,8 +1078,7 @@
// Add code owner from a lower level as reviewer.
gApi.changes().id(changeId).addReviewer(user2.email());
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
// The expected status is APPROVED since 'user' which is configured as code owner on the root
// level approved the change.
assertThatCollection(fileCodeOwnerStatuses)
@@ -959,8 +1107,7 @@
.getChangeId();
// Without Owners-Override approval the expected status is INSUFFICIENT_REVIEWERS.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
@@ -970,7 +1117,7 @@
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
// With Owners-Override approval the expected status is APPROVED.
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.APPROVED),
@@ -1002,8 +1149,7 @@
.getChangeId();
// Without override approval the expected status is INSUFFICIENT_REVIEWERS.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
@@ -1013,7 +1159,7 @@
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
// With override approval the expected status is APPROVED.
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.APPROVED),
@@ -1023,7 +1169,7 @@
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 0));
// Without override approval the expected status is INSUFFICIENT_REVIEWERS.
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
@@ -1033,7 +1179,7 @@
gApi.changes().id(changeId).current().review(new ReviewInput().label("Another-Override", 1));
// With override approval the expected status is APPROVED.
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.APPROVED),
@@ -1166,9 +1312,7 @@
gApi.projects().name(project.get()).deleteBranches(input);
ResourceConflictException exception =
- assertThrows(
- ResourceConflictException.class,
- () -> codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId)));
+ assertThrows(ResourceConflictException.class, () -> getFileCodeOwnerStatuses(changeId));
assertThat(exception).hasMessageThat().isEqualTo("destination branch not found");
}
@@ -1186,8 +1330,7 @@
.getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -1203,7 +1346,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
@@ -1223,7 +1366,7 @@
.isEqualTo(user.email());
// Check that the file is still approved.
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -1245,8 +1388,7 @@
.getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -1256,7 +1398,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
@@ -1275,7 +1417,7 @@
.isEqualTo(admin.email());
// Check that the file is still approved.
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -1294,8 +1436,7 @@
.getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -1311,7 +1452,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -1346,8 +1487,7 @@
.getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -1358,7 +1498,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -1376,8 +1516,7 @@
.getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -1393,7 +1532,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -1418,8 +1557,7 @@
createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
.getChangeId();
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -1445,8 +1583,7 @@
.getChangeId();
// Verify that the status of the file is INSUFFICIENT_REVIEWERS.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -1456,7 +1593,7 @@
// Check that the status of the file is PENDING now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
@@ -1471,7 +1608,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@@ -1491,7 +1628,7 @@
// Check that the file is not approved.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeIdOfRevert));
+ getFileCodeOwnerStatuses(changeIdOfRevert);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.deletion(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -1513,7 +1650,7 @@
// Check that the file is approved since it's a pure revert.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeIdOfRevert));
+ getFileCodeOwnerStatuses(changeIdOfRevert);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.deletion(path, CodeOwnerStatus.APPROVED));
}
@@ -1549,7 +1686,7 @@
// Check that the file is not approved.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(revertChange.changeId));
+ getFileCodeOwnerStatuses(revertChange.changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.modification(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -1569,8 +1706,7 @@
// Check that the file is approved since the uploader is exempted from requiring code owner
// approvals.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
@@ -1580,12 +1716,18 @@
// Check that the file is no longer approved since the uploader is not exempted from requiring
// code owner approvals.
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
+ private ImmutableSet<FileCodeOwnerStatus> getFileCodeOwnerStatuses(String changeId)
+ throws Exception {
+ return codeOwnerApprovalCheck.getFileStatusesAsSet(
+ getChangeNotes(changeId), /* start= */ 0, /* limit= */ 0);
+ }
+
private ChangeNotes getChangeNotes(String changeId) throws Exception {
return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
index dc87ef7..08052d8 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
@@ -76,8 +76,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -92,7 +91,7 @@
// Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
// defined).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -107,7 +106,7 @@
// Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
// defined).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -130,8 +129,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -157,8 +155,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -173,7 +170,7 @@
// Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
// defined).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -188,7 +185,7 @@
// Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
// defined).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -215,8 +212,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -235,8 +231,7 @@
// Verify that the file is not approved yet (the change owner is a code owner, but
// implicit approvals are disabled).
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -250,7 +245,7 @@
gApi.changes().id(changeId).addReviewer(user.email());
// Verify that the status is pending now .
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -264,7 +259,7 @@
recommend(changeId);
// Verify that the status is approved now
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -283,8 +278,7 @@
// Verify that the file is approved (the change owner is a code owner and implicit approvals are
// enabled).
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -310,8 +304,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -326,7 +319,7 @@
// Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
// defined).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -341,7 +334,7 @@
// Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
// defined).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -367,8 +360,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -395,8 +387,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -411,7 +402,7 @@
// Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
// defined).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -426,7 +417,7 @@
// Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
// defined).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -453,8 +444,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -486,8 +476,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -502,7 +491,7 @@
// Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
// defined).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -517,7 +506,7 @@
// Verify that the file is not approved (fallback code owner doesn't apply since a code owner is
// defined).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -550,8 +539,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -562,6 +550,12 @@
.isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
}
+ private ImmutableSet<FileCodeOwnerStatus> getFileCodeOwnerStatuses(String changeId)
+ throws Exception {
+ return codeOwnerApprovalCheck.getFileStatusesAsSet(
+ getChangeNotes(changeId), /* start= */ 0, /* limit= */ 0);
+ }
+
private ChangeNotes getChangeNotes(String changeId) throws Exception {
return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
index d17d51b..68382f9 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
@@ -83,8 +83,7 @@
.getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -105,7 +104,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -138,8 +137,7 @@
createChange(bot, "Change Adding A File", JgitPath.of(path).get(), "file content")
.getChangeId();
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -167,8 +165,7 @@
.getChangeId();
// Verify that the status of the file is INSUFFICIENT_REVIEWERS.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -183,7 +180,7 @@
// Check that the status of the file is PENDING now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -203,7 +200,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -224,8 +221,7 @@
// Verify that the file is not approved yet (the change owner is a global code owner, but
// implicit approvals are disabled).
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -240,7 +236,7 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -272,8 +268,7 @@
String changeId =
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -297,8 +292,7 @@
// Verify that the status of the file is INSUFFICIENT_REVIEWERS (since there is no implicit
// approval by default).
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -312,7 +306,7 @@
gApi.changes().id(changeId).addReviewer(user.email());
// Check that the status of the file is PENDING now.
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -341,8 +335,7 @@
requestScopeOperations.setApiUser(user3.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -373,8 +366,7 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -400,8 +392,7 @@
requestScopeOperations.setApiUser(admin.id());
recommend(changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -437,8 +428,7 @@
// Amend change with a user that is a project owner.
amendChange(admin, changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -466,8 +456,7 @@
// Amend change with a user that is not a project owner.
amendChange(user, changeId);
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -501,8 +490,7 @@
.getChangeId();
// Without Owners-Override approval the expected status is INSUFFICIENT_REVIEWERS.
- for (FileCodeOwnerStatus fileCodeOwnerStatus :
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId))) {
+ for (FileCodeOwnerStatus fileCodeOwnerStatus : getFileCodeOwnerStatuses(changeId)) {
assertThat(fileCodeOwnerStatus)
.hasNewPathStatus()
.value()
@@ -514,8 +502,7 @@
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
// With Owners-Override approval the expected status is APPROVED.
- for (FileCodeOwnerStatus fileCodeOwnerStatus :
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId))) {
+ for (FileCodeOwnerStatus fileCodeOwnerStatus : getFileCodeOwnerStatuses(changeId)) {
assertThat(fileCodeOwnerStatus)
.hasNewPathStatus()
.value()
@@ -547,8 +534,7 @@
.getChangeId();
// Without override approval the expected status is INSUFFICIENT_REVIEWERS.
- for (FileCodeOwnerStatus fileCodeOwnerStatus :
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId))) {
+ for (FileCodeOwnerStatus fileCodeOwnerStatus : getFileCodeOwnerStatuses(changeId)) {
assertThat(fileCodeOwnerStatus)
.hasNewPathStatus()
.value()
@@ -561,8 +547,7 @@
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
// With override approval the expected status is APPROVED.
- for (FileCodeOwnerStatus fileCodeOwnerStatus :
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId))) {
+ for (FileCodeOwnerStatus fileCodeOwnerStatus : getFileCodeOwnerStatuses(changeId)) {
assertThat(fileCodeOwnerStatus)
.hasNewPathStatus()
.value()
@@ -574,8 +559,7 @@
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 0));
// Without override approval the expected status is INSUFFICIENT_REVIEWERS.
- for (FileCodeOwnerStatus fileCodeOwnerStatus :
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId))) {
+ for (FileCodeOwnerStatus fileCodeOwnerStatus : getFileCodeOwnerStatuses(changeId)) {
assertThat(fileCodeOwnerStatus)
.hasNewPathStatus()
.value()
@@ -587,8 +571,7 @@
gApi.changes().id(changeId).current().review(new ReviewInput().label("Another-Override", 1));
// With override approval the expected status is APPROVED.
- for (FileCodeOwnerStatus fileCodeOwnerStatus :
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId))) {
+ for (FileCodeOwnerStatus fileCodeOwnerStatus : getFileCodeOwnerStatuses(changeId)) {
assertThat(fileCodeOwnerStatus)
.hasNewPathStatus()
.value()
@@ -610,8 +593,7 @@
.getChangeId();
// Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -621,7 +603,7 @@
approve(changeId);
// Verify that the file is not approved yet
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -637,11 +619,17 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
+ private ImmutableSet<FileCodeOwnerStatus> getFileCodeOwnerStatuses(String changeId)
+ throws Exception {
+ return codeOwnerApprovalCheck.getFileStatusesAsSet(
+ getChangeNotes(changeId), /* start= */ 0, /* limit= */ 0);
+ }
+
private ChangeNotes getChangeNotes(String changeId) throws Exception {
return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
index a27bde9..6cdf2b1 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
@@ -83,8 +83,7 @@
.getChangeId();
// Verify that the file is not approved.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -99,7 +98,7 @@
recommend(changeId);
// Verify that the file is not approved (since self approvals are ignored).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -125,8 +124,7 @@
amendChange(admin, changeId);
// Verify that the file is not approved.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -142,7 +140,7 @@
// Verify that the file is approved now (since the change owner is not the uploader of the
// current patch set).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -172,8 +170,7 @@
amendChange(codeOwner, changeId);
// Verify that the file is not approved.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -188,7 +185,7 @@
// Verify that the file is not pending (the code owner is the uploader of the current patch set
// and self approvals are ignored).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -203,7 +200,7 @@
// Verify that the file is not approved (since the code owner is the uploader of the current
// patch set and self approvals are ignored).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -227,8 +224,7 @@
.getChangeId();
// Verify that the file is not approved.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -260,8 +256,7 @@
amendChange(codeOwner, changeId);
// Verify that the file is not approved.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -286,8 +281,7 @@
.getChangeId();
// Verify that the file is approved.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -319,8 +313,7 @@
amendChange(codeOwner, changeId);
// Verify that the file is approved.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -343,8 +336,7 @@
.getChangeId();
// Verify that the file is not approved.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -360,7 +352,7 @@
// Verify that the file is not approved (since self approvals on the override label are
// ignored).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -385,8 +377,7 @@
amendChange(admin, changeId);
// Verify that the file is not approved.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -402,7 +393,7 @@
// Verify that the file is approved now (since the change owner is not the uploader of the
// current patch set and hence the override counts).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -427,8 +418,7 @@
amendChange(admin, changeId);
// Verify that the file is not approved.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
@@ -442,7 +432,7 @@
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
// Verify that the file is not approved (since the override from the uploader is ignored).
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
fileCodeOwnerStatusSubject
@@ -452,6 +442,12 @@
.isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
}
+ private ImmutableSet<FileCodeOwnerStatus> getFileCodeOwnerStatuses(String changeId)
+ throws Exception {
+ return codeOwnerApprovalCheck.getFileStatusesAsSet(
+ getChangeNotes(changeId), /* start= */ 0, /* limit= */ 0);
+ }
+
private ChangeNotes getChangeNotes(String changeId) throws Exception {
return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
}
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 93c6a3b..0c4ccf5 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -27,6 +27,8 @@
approval is applied.
* `get_auto_merge`:
Latency for getting the auto merge commit of a merge commit.
+* `get_changed_files`:
+ Latency for getting changed files from diff cache.
* `prepare_file_status_computation`:
Latency for preparing the file status computation.
* `prepare_file_status_computation_for_account`:
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index be963ff..461232e 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -604,6 +604,13 @@
The code owner statuses are always listed for the files in the current revision
of the change (latest patch set).
+The following request parameters can be specified:
+
+| Field Name | | Description |
+| ------------ | --------- | ----------- |
+| `start`\|`S` | optional | Number of file code owner statuses to skip. Allows to page over the file code owner statuses. By default 0.
+| `limit`\|`n` | optional | Limit defining how many file code owner statuses should be returned at most. By default 0 (= unlimited).
+
The code owner statuses are returned as a
[CodeOwnerStatusInfo](#code-owner-status-info) entity.
@@ -939,10 +946,11 @@
The `CodeOwnerStatusInfo` entity describes the code owner statuses for the files
in a change.
-| Field Name | Description |
-| ------------------ | ----------- |
-| `patch_set_number` | The number of the patch set for which the code owner statuses are returned.
-| `file_code_owner_statuses` | List of the code owner statuses for the files in the change as [FileCodeOwnerStatusInfo](#file-code-owner-status-info) entities, sorted by new path, then old path.
+| Field Name | | Description |
+| ------------------ | -------- | ----------- |
+| `patch_set_number` | | The number of the patch set for which the code owner statuses are returned.
+| `file_code_owner_statuses` | | List of the code owner statuses for the files in the change as [FileCodeOwnerStatusInfo](#file-code-owner-status-info) entities, sorted by new path, then old path.
+| `more` | optional | Whether the request would deliver more results if not limited. Not set if `false`.
### <a id="code-owners-status-info"> CodeOwnersStatusInfo
The `CodeOwnersStatusInfo` contains information about whether the code owners
diff --git a/ui/BUILD b/ui/BUILD
new file mode 100644
index 0000000..274676e
--- /dev/null
+++ b/ui/BUILD
@@ -0,0 +1,14 @@
+load("//tools/bzl:js.bzl", "gerrit_js_bundle")
+
+package_group(
+ name = "visibility",
+ packages = ["//plugins/code-owners/..."],
+)
+
+package(default_visibility = [":visibility"])
+
+gerrit_js_bundle(
+ name = "code-owners",
+ srcs = glob(["*.js"]),
+ entry_point = "plugin.js",
+)
diff --git a/ui/suggest-owners.js b/ui/suggest-owners.js
index 2304523..f07003f 100644
--- a/ui/suggest-owners.js
+++ b/ui/suggest-owners.js
@@ -105,9 +105,6 @@
padding: 0 var(--spacing-m);
margin: var(--spacing-m) 0;
}
- p.loading {
- text-align: center;
- }
.loadingSpin {
display: inline-block;
}
@@ -130,12 +127,17 @@
}
.suggestion-row {
flex-wrap: wrap;
- border-bottom: 1px solid var(--border-color);
+ border-top: 1px solid var(--border-color);
padding: var(--spacing-s) 0;
}
.show-all-owners-row {
- padding: var(--spacing-m) var(--spacing-xl) var(--spacing-s);
- justify-content: flex-end;
+ padding: var(--spacing-m) var(--spacing-xl) var(--spacing-s) 0;
+ }
+ .show-all-owners-row .loading {
+ padding: 0;
+ }
+ .show-all-owners-row .show-all-label {
+ margin-left: auto; /* align label to the right */
}
.suggestion-row-indicator {
margin-right: var(--spacing-s);
@@ -247,10 +249,22 @@
margin-right: var(--spacing-m);
}
</style>
- <p class="loading" hidden="[[!isLoading]]">
- <span class="loadingSpin"></span>
- [[progressText]]
- </p>
+ <ul class="suggestion-container">
+ <li class="show-all-owners-row">
+ <p class="loading" hidden="[[!isLoading]]">
+ <span class="loadingSpin"></span>
+ [[progressText]]
+ </p>
+ <label class="show-all-label">
+ <input
+ id="showAllOwnersCheckbox"
+ type="checkbox"
+ checked="{{_showAllOwners::change}}"
+ />
+ Show all owners
+ </label>
+ </li>
+ </ul>
<ul class="suggestion-container">
<template
is="dom-repeat"
@@ -332,16 +346,6 @@
</template>
</li>
</template>
- <li class="show-all-owners-row">
- <label>
- <input
- id="showAllOwnersCheckbox"
- type="checkbox"
- checked="{{_showAllOwners::change}}"
- />
- Show all owners
- </label>
- </li>
</ul>
`;
}
