Merge "TransientCodeOwnerConfigCache: Log warning if the limit has been reached"
diff --git a/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java
index eabf850..cd8aacb 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java
@@ -23,8 +23,8 @@
* <p>To create an instance for a change use {@code ChangeCodeOwnersFactory}.
*/
public interface ChangeCodeOwners {
- /** Returns the code owner status for the files in the change. */
- CodeOwnerStatusInfo getCodeOwnerStatus() throws RestApiException;
+ /** Creates a request to retrieve the code owner status for the files in the change. */
+ CodeOwnerStatusRequest getCodeOwnerStatus() throws RestApiException;
/** Returns the revision-level code owners API for the current revision. */
default RevisionCodeOwners current() throws RestApiException {
@@ -35,12 +35,26 @@
RevisionCodeOwners revision(String id) throws RestApiException;
/**
+ * Request to compute code owner status.
+ *
+ * <p>Allows to set parameters on the request before executing it by calling {@link #get()}.
+ */
+ abstract class CodeOwnerStatusRequest {
+ /**
+ * Executes this request and retrieves the code owner status.
+ *
+ * @return the code owner status
+ */
+ public abstract CodeOwnerStatusInfo get() throws RestApiException;
+ }
+
+ /**
* A default implementation which allows source compatibility when adding new methods to the
* interface.
*/
class NotImplemented implements ChangeCodeOwners {
@Override
- public CodeOwnerStatusInfo getCodeOwnerStatus() {
+ public CodeOwnerStatusRequest getCodeOwnerStatus() {
throw new NotImplementedException();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java
index 578c1c3..655ef66 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java
@@ -48,6 +48,7 @@
private String revision;
private Long seed;
private Boolean resolveAllUsers;
+ private Boolean highestScoreOnly;
/**
* Lists the code owners for the given path.
@@ -116,6 +117,17 @@
}
/**
+ * Sets whether only the code owners with the highest score should be returned.
+ *
+ * @param highestScoreOnly whether only the code owners with the highest score should be
+ * returned
+ */
+ public QueryRequest withHighestScoreOnly(boolean highestScoreOnly) {
+ this.highestScoreOnly = highestScoreOnly;
+ return this;
+ }
+
+ /**
* Sets the branch revision from which the code owner configs should be read.
*
* <p>Not supported for querying code owners for a path in a change.
@@ -149,6 +161,11 @@
return Optional.ofNullable(resolveAllUsers);
}
+ /** Whether only the code owners with the highest score should be returned. */
+ public Optional<Boolean> getHighestScoreOnly() {
+ return Optional.ofNullable(highestScoreOnly);
+ }
+
/** Returns the branch revision from which the code owner configs should be read. */
public Optional<String> getRevision() {
return Optional.ofNullable(revision);
diff --git a/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java b/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java
index 0fc1497..ba41418 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java
@@ -31,4 +31,11 @@
* <p>The paths are sorted alphabetically.
*/
public List<String> ownedPaths;
+
+ /**
+ * 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/RevisionCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/RevisionCodeOwners.java
index 1b90340..7b2655b 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/RevisionCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/RevisionCodeOwners.java
@@ -20,6 +20,7 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
/**
* Revision-level Java API of the code-owners plugin.
@@ -95,8 +96,40 @@
/** Request to check code owner config files. */
abstract class OwnedPathsRequest {
+ private Integer start;
+ private Integer limit;
private String user;
+ /**
+ * Sets a limit on the number of owned paths that should be returned.
+ *
+ * @param start number of owned paths to skip
+ */
+ public OwnedPathsRequest withStart(int start) {
+ this.start = start;
+ return this;
+ }
+
+ /** Returns the number of owned paths to skip. */
+ public Optional<Integer> getStart() {
+ return Optional.ofNullable(start);
+ }
+
+ /**
+ * Sets a limit on the number of owned paths that should be returned.
+ *
+ * @param limit the limit
+ */
+ public OwnedPathsRequest withLimit(int limit) {
+ this.limit = limit;
+ return this;
+ }
+
+ /** Returns the limit. */
+ public Optional<Integer> getLimit() {
+ return Optional.ofNullable(limit);
+ }
+
/** Sets the user for which the owned paths should be retrieved. */
public OwnedPathsRequest forUser(String user) {
this.user = user;
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 9f266f8..5a20f34 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/ChangeCodeOwnersImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/ChangeCodeOwnersImpl.java
@@ -52,12 +52,17 @@
}
@Override
- public CodeOwnerStatusInfo getCodeOwnerStatus() throws RestApiException {
- try {
- return getCodeOwnerStatus.apply(changeResource).value();
- } catch (Exception e) {
- throw asRestApiException("Cannot get code owner status", e);
- }
+ public CodeOwnerStatusRequest getCodeOwnerStatus() throws RestApiException {
+ return new CodeOwnerStatusRequest() {
+ @Override
+ public CodeOwnerStatusInfo get() throws RestApiException {
+ try {
+ return getCodeOwnerStatus.apply(changeResource).value();
+ } catch (Exception e) {
+ throw asRestApiException("Cannot get code owner status", e);
+ }
+ }
+ };
}
@Override
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java
index 9fba599..107fa61 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java
@@ -59,6 +59,7 @@
getLimit().ifPresent(getCodeOwners::setLimit);
getSeed().ifPresent(getCodeOwners::setSeed);
getResolveAllUsers().ifPresent(getCodeOwners::setResolveAllUsers);
+ getHighestScoreOnly().ifPresent(getCodeOwners::setHighestScoreOnly);
getRevision().ifPresent(getCodeOwners::setRevision);
CodeOwnersInBranchCollection.PathResource pathInBranchResource =
codeOwnersInBranchCollection.parse(
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java
index ed2bad9..b185172 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java
@@ -64,6 +64,7 @@
getLimit().ifPresent(getCodeOwners::setLimit);
getSeed().ifPresent(getCodeOwners::setSeed);
getResolveAllUsers().ifPresent(getCodeOwners::setResolveAllUsers);
+ getHighestScoreOnly().ifPresent(getCodeOwners::setHighestScoreOnly);
CodeOwnersInChangeCollection.PathResource pathInChangeResource =
codeOwnersInChangeCollection.parse(
revisionResource, IdString.fromDecoded(path.toString()));
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/RevisionCodeOwnersImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/RevisionCodeOwnersImpl.java
index d42327c..db8cb27 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/RevisionCodeOwnersImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/RevisionCodeOwnersImpl.java
@@ -75,6 +75,8 @@
public OwnedPathsInfo get() throws RestApiException {
try {
GetOwnedPaths getOwnedPaths = getOwnedPathsProvider.get();
+ getStart().ifPresent(getOwnedPaths::setStart);
+ getLimit().ifPresent(getOwnedPaths::setLimit);
getOwnedPaths.setUser(getUser());
return getOwnedPaths.apply(revisionResource).value();
} catch (Exception e) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index af7d9b7..f98ecb8 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -121,21 +121,23 @@
* @param changeNotes the change notes for which the owned files should be returned
* @param patchSet the patch set for which the owned files should be returned
* @param accountId account ID of the code owner for which the owned files should be returned
+ * @param start number of owned paths to skip
* @param limit the max number of owned paths that should be returned (0 = unlimited)
* @return the paths of the files in the given patch set that are owned by the specified account
* @throws ResourceConflictException if the destination branch of the change no longer exists
*/
public ImmutableList<Path> getOwnedPaths(
- ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int limit)
+ ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int start, int limit)
throws ResourceConflictException {
try (Timer0.Context ctx = codeOwnerMetrics.computeOwnedPaths.start()) {
logger.atFine().log(
"compute owned paths for account %d (project = %s, change = %d, patch set = %d,"
- + " limit = %d)",
+ + " start = %d, limit = %d)",
accountId.get(),
changeNotes.getProjectName(),
changeNotes.getChangeId().get(),
patchSet.id().get(),
+ start,
limit);
Stream<Path> ownedPaths =
getFileStatusesForAccount(changeNotes, patchSet, accountId)
@@ -149,6 +151,9 @@
.filter(
pathCodeOwnerStatus -> pathCodeOwnerStatus.status() == CodeOwnerStatus.APPROVED)
.map(PathCodeOwnerStatus::path);
+ if (start > 0) {
+ ownedPaths = ownedPaths.skip(start);
+ }
if (limit > 0) {
ownedPaths = ownedPaths.limit(limit);
}
@@ -330,8 +335,7 @@
branch,
revision,
globalCodeOwners,
- enableImplicitApprovalFromUploader,
- patchSetUploader,
+ enableImplicitApprovalFromUploader ? patchSetUploader : null,
reviewerAccountIds,
approverAccountIds,
fallbackCodeOwners,
@@ -400,8 +404,7 @@
// should be ignored. For the given account we do not need to check for
// implicit approvals since all owned files are already covered by the
// explicit approval.
- /* enableImplicitApprovalFromUploader= */ false,
- /* patchSetUploader= */ null,
+ /* implicitApprover= */ null,
/* reviewerAccountIds= */ ImmutableSet.of(),
// Assume an explicit approval of the given account.
/* approverAccountIds= */ ImmutableSet.of(accountId),
@@ -449,8 +452,7 @@
BranchNameKey branch,
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
- boolean enableImplicitApprovalFromUploader,
- @Nullable Account.Id patchSetUploader,
+ @Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
FallbackCodeOwners fallbackCodeOwners,
@@ -470,8 +472,7 @@
branch,
revision,
globalCodeOwners,
- enableImplicitApprovalFromUploader,
- patchSetUploader,
+ implicitApprover,
reviewerAccountIds,
approverAccountIds,
fallbackCodeOwners,
@@ -493,8 +494,7 @@
branch,
revision,
globalCodeOwners,
- enableImplicitApprovalFromUploader,
- patchSetUploader,
+ implicitApprover,
reviewerAccountIds,
approverAccountIds,
fallbackCodeOwners,
@@ -514,8 +514,7 @@
BranchNameKey branch,
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
- boolean enableImplicitApprovalFromUploader,
- @Nullable Account.Id patchSetUploader,
+ @Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
FallbackCodeOwners fallbackCodeOwners,
@@ -533,12 +532,7 @@
AtomicReference<CodeOwnerStatus> codeOwnerStatus =
new AtomicReference<>(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- if (isApproved(
- absolutePath,
- globalCodeOwners,
- approverAccountIds,
- enableImplicitApprovalFromUploader,
- patchSetUploader)) {
+ if (isApproved(absolutePath, globalCodeOwners, approverAccountIds, implicitApprover)) {
logger.atFine().log("%s was approved by a global code owner", absolutePath);
codeOwnerStatus.set(CodeOwnerStatus.APPROVED);
} else {
@@ -568,12 +562,7 @@
hasRevelantCodeOwnerDefinitions.set(true);
}
- if (isApproved(
- absolutePath,
- codeOwners,
- approverAccountIds,
- enableImplicitApprovalFromUploader,
- patchSetUploader)) {
+ if (isApproved(absolutePath, codeOwners, approverAccountIds, implicitApprover)) {
codeOwnerStatus.set(CodeOwnerStatus.APPROVED);
return false;
} else if (isPending(absolutePath, codeOwners, reviewerAccountIds)) {
@@ -606,8 +595,7 @@
codeOwnerStatus.get(),
branch,
globalCodeOwners,
- enableImplicitApprovalFromUploader,
- patchSetUploader,
+ implicitApprover,
reviewerAccountIds,
approverAccountIds,
fallbackCodeOwners,
@@ -628,8 +616,7 @@
private CodeOwnerStatus getCodeOwnerStatusForProjectOwnersAsFallbackCodeOwners(
BranchNameKey branch,
CodeOwnerResolverResult globalCodeOwners,
- boolean enableImplicitApprovalFromUploader,
- @Nullable Account.Id patchSetUploader,
+ @Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
Path absolutePath) {
@@ -639,12 +626,7 @@
CodeOwnerStatus codeOwnerStatus = CodeOwnerStatus.INSUFFICIENT_REVIEWERS;
if (isApprovedByProjectOwnerOrGlobalOwner(
- branch.project(),
- absolutePath,
- globalCodeOwners,
- approverAccountIds,
- enableImplicitApprovalFromUploader,
- patchSetUploader)) {
+ branch.project(), absolutePath, globalCodeOwners, approverAccountIds, implicitApprover)) {
codeOwnerStatus = CodeOwnerStatus.APPROVED;
} else if (isPendingByProjectOwnerOrGlobalOwner(
branch.project(), absolutePath, globalCodeOwners, reviewerAccountIds)) {
@@ -660,11 +642,10 @@
Path absolutePath,
CodeOwnerResolverResult globalCodeOwners,
ImmutableSet<Account.Id> approverAccountIds,
- boolean enableImplicitApprovalFromUploader,
- @Nullable Account.Id patchSetUploader) {
- return (enableImplicitApprovalFromUploader
+ @Nullable Account.Id implicitApprover) {
+ return (implicitApprover != null
&& isImplicitlyApprovedByProjectOwnerOrGlobalOwner(
- projectName, absolutePath, globalCodeOwners, patchSetUploader))
+ projectName, absolutePath, globalCodeOwners, implicitApprover))
|| isExplicitlyApprovedByProjectOwnerOrGlobalOwner(
projectName, absolutePath, globalCodeOwners, approverAccountIds);
}
@@ -673,10 +654,9 @@
Project.NameKey projectName,
Path absolutePath,
CodeOwnerResolverResult globalCodeOwners,
- Account.Id patchSetUploader) {
- requireNonNull(
- patchSetUploader, "patchSetUploader must be set if implicit approvals are enabled");
- if (isProjectOwner(projectName, patchSetUploader)) {
+ Account.Id implicitApprover) {
+ requireNonNull(implicitApprover, "implicitApprover");
+ if (isProjectOwner(projectName, implicitApprover)) {
// The uploader of the patch set is a project owner and thus a code owner. This means there
// is an implicit code owner approval from the patch set uploader so that the path is
// automatically approved.
@@ -687,7 +667,7 @@
}
if (globalCodeOwners.ownedByAllUsers()
- || globalCodeOwners.codeOwnersAccountIds().contains(patchSetUploader)) {
+ || globalCodeOwners.codeOwnersAccountIds().contains(implicitApprover)) {
// If the uploader of the patch set is a global code owner, there is an implicit code owner
// approval from the patch set uploader so that the path is automatically approved.
logger.atFine().log(
@@ -749,8 +729,7 @@
CodeOwnerStatus codeOwnerStatus,
BranchNameKey branch,
CodeOwnerResolverResult globalCodeOwners,
- boolean enableImplicitApprovalFromUploader,
- @Nullable Account.Id patchSetUploader,
+ @Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
FallbackCodeOwners fallbackCodeOwners,
@@ -766,17 +745,13 @@
return getCodeOwnerStatusForProjectOwnersAsFallbackCodeOwners(
branch,
globalCodeOwners,
- enableImplicitApprovalFromUploader,
- patchSetUploader,
+ implicitApprover,
reviewerAccountIds,
approverAccountIds,
absolutePath);
case ALL_USERS:
return getCodeOwnerStatusIfAllUsersAreCodeOwners(
- enableImplicitApprovalFromUploader,
- reviewerAccountIds,
- approverAccountIds,
- absolutePath);
+ implicitApprover != null, reviewerAccountIds, approverAccountIds, absolutePath);
}
throw new CodeOwnersInternalServerErrorException(
@@ -816,12 +791,9 @@
Path absolutePath,
CodeOwnerResolverResult codeOwners,
ImmutableSet<Account.Id> approverAccountIds,
- boolean enableImplicitApprovalFromUploader,
- @Nullable Account.Id patchSetUploader) {
- if (enableImplicitApprovalFromUploader) {
- requireNonNull(
- patchSetUploader, "patchSetUploader must be set if implicit approvals are enabled");
- if (codeOwners.codeOwnersAccountIds().contains(patchSetUploader)
+ @Nullable Account.Id implicitApprover) {
+ if (implicitApprover != null) {
+ if (codeOwners.codeOwnersAccountIds().contains(implicitApprover)
|| codeOwners.ownedByAllUsers()) {
// If the uploader of the patch set owns the path, there is an implicit code owner
// approval from the patch set uploader so that the path is automatically approved.
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
index 84a263b..84bddff 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
@@ -14,11 +14,13 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+
import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
-import java.util.Comparator;
-import java.util.HashSet;
import java.util.Set;
+import java.util.function.Function;
import java.util.stream.Collectors;
/**
@@ -40,21 +42,14 @@
return new AutoValue_CodeOwnerScorings(ImmutableSet.copyOf(codeOwnerScorings));
}
- public static CodeOwnerScorings appendScoring(
- CodeOwnerScorings codeOwnerScorings, CodeOwnerScoring codeOwnerScoringToBeAdded) {
- Set<CodeOwnerScoring> codeOwnerScoringSet = new HashSet<>(codeOwnerScorings.scorings());
- codeOwnerScoringSet.add(codeOwnerScoringToBeAdded);
- return create(codeOwnerScoringSet);
- }
-
/**
- * Returns a comparator to sort code owners by the collected scorings.
+ * Returns the total scorings for the given code owners.
*
- * <p>Code owners with higher scoring come first. The order of code owners with the same scoring
- * is undefined.
+ * @param codeOwners the code owners for which the scorings should be returned
*/
- public Comparator<CodeOwner> comparingByScorings() {
- return Comparator.comparingDouble(this::sumWeightedScorings).reversed();
+ public ImmutableMap<CodeOwner, Double> getScorings(ImmutableSet<CodeOwner> codeOwners) {
+ return codeOwners.stream()
+ .collect(toImmutableMap(Function.identity(), this::sumWeightedScorings));
}
/** Returns the sum of all weighted scorings that available for the given code owner. */
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index c35d1b0..011e8cf 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -100,6 +100,11 @@
changeData.currentPatchSet().id().get(), changeData.change().getId().get()));
return Optional.of(notReady());
} catch (Throwable t) {
+ // Whether the exception should be treated as RULE_ERROR.
+ // RULE_ERROR must only be returned if the exception is caused by user misconfiguration (e.g.
+ // an invalid OWNERS file), but not for internal server errors.
+ boolean isRuleError = false;
+
String cause = t.getClass().getSimpleName();
String errorMessage = "Failed to evaluate code owner statuses";
if (changeData != null) {
@@ -113,9 +118,11 @@
Optional<InvalidCodeOwnerConfigException> invalidCodeOwnerConfigException =
CodeOwners.getInvalidCodeOwnerConfigCause(t);
if (invalidPathException.isPresent()) {
+ isRuleError = true;
cause = "invalid_path";
errorMessage += String.format(" (cause: %s)", invalidPathException.get().getMessage());
} else if (invalidCodeOwnerConfigException.isPresent()) {
+ isRuleError = true;
codeOwnerMetrics.countInvalidCodeOwnerConfigFiles.increment(
invalidCodeOwnerConfigException.get().getProjectName().get(),
invalidCodeOwnerConfigException.get().getRef(),
@@ -137,7 +144,11 @@
errorMessage += ".";
logger.atSevere().withCause(t).log(errorMessage);
codeOwnerMetrics.countCodeOwnerSubmitRuleErrors.increment(cause);
- return Optional.of(ruleError(errorMessage));
+
+ if (isRuleError) {
+ return Optional.of(ruleError(errorMessage));
+ }
+ throw new CodeOwnersInternalServerErrorException(errorMessage, t);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index e6a55ee..403fe49 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -164,7 +164,11 @@
// limit + 1, so that we can show an indicator if there are more than <limit> files.
ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes, changeNotes.getCurrentPatchSet(), reviewerAccountId, limit + 1);
+ changeNotes,
+ changeNotes.getCurrentPatchSet(),
+ reviewerAccountId,
+ /* start= */ 0,
+ limit + 1);
} catch (RestApiException e) {
logger.atFine().withCause(e).log(
"Couldn't compute owned paths of change %s for account %s",
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/FileCodeOwnerStatus.java b/java/com/google/gerrit/plugins/codeowners/backend/FileCodeOwnerStatus.java
index 34ff019..330917d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/FileCodeOwnerStatus.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/FileCodeOwnerStatus.java
@@ -14,8 +14,13 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static java.util.Objects.requireNonNull;
+
import com.google.auto.value.AutoValue;
import com.google.gerrit.plugins.codeowners.common.ChangedFile;
+import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
+import com.google.gerrit.plugins.codeowners.util.JgitPath;
+import java.nio.file.Path;
import java.util.Optional;
/** Code owner status for a particular file that was changed in a change. */
@@ -56,4 +61,77 @@
return new AutoValue_FileCodeOwnerStatus(
changedFile, newPathCodeOwnerStatus, oldPathCodeOwnerStatus);
}
+
+ public static FileCodeOwnerStatus addition(String path, CodeOwnerStatus codeOwnerStatus) {
+ requireNonNull(path, "path");
+
+ return addition(JgitPath.of(path).getAsAbsolutePath(), codeOwnerStatus);
+ }
+
+ public static FileCodeOwnerStatus addition(Path path, CodeOwnerStatus codeOwnerStatus) {
+ requireNonNull(path, "path");
+ requireNonNull(codeOwnerStatus, "codeOwnerStatus");
+
+ return create(
+ ChangedFile.addition(path),
+ Optional.of(PathCodeOwnerStatus.create(path, codeOwnerStatus)),
+ Optional.empty());
+ }
+
+ public static FileCodeOwnerStatus modification(Path path, CodeOwnerStatus codeOwnerStatus) {
+ requireNonNull(path, "path");
+ requireNonNull(codeOwnerStatus, "codeOwnerStatus");
+
+ return create(
+ ChangedFile.modification(path),
+ Optional.of(PathCodeOwnerStatus.create(path, codeOwnerStatus)),
+ Optional.empty());
+ }
+
+ public static FileCodeOwnerStatus deletion(String path, CodeOwnerStatus codeOwnerStatus) {
+ requireNonNull(path, "path");
+
+ return deletion(JgitPath.of(path).getAsAbsolutePath(), codeOwnerStatus);
+ }
+
+ public static FileCodeOwnerStatus deletion(Path path, CodeOwnerStatus codeOwnerStatus) {
+ requireNonNull(path, "path");
+ requireNonNull(codeOwnerStatus, "codeOwnerStatus");
+
+ return create(
+ ChangedFile.deletion(path),
+ Optional.empty(),
+ Optional.of(PathCodeOwnerStatus.create(path, codeOwnerStatus)));
+ }
+
+ public static FileCodeOwnerStatus rename(
+ String oldPath,
+ CodeOwnerStatus oldPathCodeOwnerStatus,
+ String newPath,
+ CodeOwnerStatus newPathCodeOwnerStatus) {
+ requireNonNull(oldPath, "oldPath");
+ requireNonNull(newPath, "newPath");
+
+ return rename(
+ JgitPath.of(oldPath).getAsAbsolutePath(),
+ oldPathCodeOwnerStatus,
+ JgitPath.of(newPath).getAsAbsolutePath(),
+ newPathCodeOwnerStatus);
+ }
+
+ public static FileCodeOwnerStatus rename(
+ Path oldPath,
+ CodeOwnerStatus oldPathCodeOwnerStatus,
+ Path newPath,
+ CodeOwnerStatus newPathCodeOwnerStatus) {
+ requireNonNull(oldPath, "oldPath");
+ requireNonNull(oldPathCodeOwnerStatus, "oldPathCodeOwnerStatus");
+ requireNonNull(newPath, "newPath");
+ requireNonNull(newPathCodeOwnerStatus, "newPathCodeOwnerStatus");
+
+ return create(
+ ChangedFile.rename(newPath, oldPath),
+ Optional.of(PathCodeOwnerStatus.create(newPath, newPathCodeOwnerStatus)),
+ Optional.of(PathCodeOwnerStatus.create(oldPath, oldPathCodeOwnerStatus)));
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index e9e2c39..9803951 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -124,7 +124,11 @@
// limit + 1, so that we can show an indicator if there are more than <limit> files.
ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes, changeNotes.getCurrentPatchSet(), user.getAccountId(), limit + 1);
+ changeNotes,
+ changeNotes.getCurrentPatchSet(),
+ user.getAccountId(),
+ /* start= */ 0,
+ limit + 1);
} catch (RestApiException e) {
logger.atFine().withCause(e).log(
"Couldn't compute owned paths of change %s for account %s",
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnerStatus.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnerStatus.java
index 9f4b87e..b5f0976 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnerStatus.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnerStatus.java
@@ -14,8 +14,11 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static java.util.Objects.requireNonNull;
+
import com.google.auto.value.AutoValue;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
+import com.google.gerrit.plugins.codeowners.util.JgitPath;
import java.nio.file.Path;
/** Code owner status for a particular path that has been modified in a change. */
@@ -41,4 +44,18 @@
public static PathCodeOwnerStatus create(Path path, CodeOwnerStatus codeOwnerStatus) {
return new AutoValue_PathCodeOwnerStatus(path, codeOwnerStatus);
}
+
+ /**
+ * Creates a {@link PathCodeOwnerStatus} instance.
+ *
+ * @param path the path to which the code owner status belongs
+ * @param codeOwnerStatus the code owner status
+ * @return the created {@link PathCodeOwnerStatus} instance
+ */
+ public static PathCodeOwnerStatus create(String path, CodeOwnerStatus codeOwnerStatus) {
+ requireNonNull(path, "path");
+ requireNonNull(codeOwnerStatus, "codeOwnerStatus");
+
+ return create(JgitPath.of(path).getAsAbsolutePath(), codeOwnerStatus);
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java b/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java
index 3d2caec..f66c31b 100644
--- a/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java
+++ b/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java
@@ -161,4 +161,38 @@
private static Optional<Path> convertPathFromPatchListEntry(@Nullable String path) {
return Optional.ofNullable(path).map(newName -> JgitPath.of(newName).getAsAbsolutePath());
}
+
+ public static ChangedFile create(
+ Optional<String> newPath, Optional<String> oldPath, ChangeType changeType) {
+ requireNonNull(changeType, "changeType");
+
+ return new AutoValue_ChangedFile(
+ newPath.map(JgitPath::of).map(JgitPath::getAsAbsolutePath),
+ oldPath.map(JgitPath::of).map(JgitPath::getAsAbsolutePath),
+ changeType);
+ }
+
+ public static ChangedFile addition(Path newPath) {
+ requireNonNull(newPath, "newPath");
+
+ return new AutoValue_ChangedFile(Optional.of(newPath), Optional.empty(), ChangeType.ADD);
+ }
+
+ public static ChangedFile modification(Path path) {
+ requireNonNull(path, "path");
+
+ return new AutoValue_ChangedFile(Optional.of(path), Optional.of(path), ChangeType.MODIFY);
+ }
+
+ public static ChangedFile deletion(Path path) {
+ requireNonNull(path, "path");
+
+ return new AutoValue_ChangedFile(Optional.empty(), Optional.of(path), ChangeType.DELETE);
+ }
+
+ public static ChangedFile rename(Path newPath, Path oldPath) {
+ requireNonNull(newPath, "newPath");
+
+ return new AutoValue_ChangedFile(Optional.of(newPath), Optional.of(oldPath), ChangeType.RENAME);
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 0644b4c..d1e6cd2 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -19,6 +19,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
@@ -53,6 +54,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.Comparator;
import java.util.EnumSet;
import java.util.HashSet;
import java.util.List;
@@ -87,6 +89,7 @@
private int limit = DEFAULT_LIMIT;
private Optional<Long> seed = Optional.empty();
private boolean resolveAllUsers;
+ private boolean highestScoreOnly;
@Option(
name = "-o",
@@ -128,6 +131,13 @@
this.resolveAllUsers = resolveAllUsers;
}
+ @Option(
+ name = "--highest-score-only",
+ usage = "whether only code owners with the highest score should be returned")
+ public void setHighestScoreOnly(boolean highestScoreOnly) {
+ this.highestScoreOnly = highestScoreOnly;
+ }
+
protected AbstractGetCodeOwnersForPath(
AccountVisibility accountVisibility,
Accounts accounts,
@@ -179,21 +189,6 @@
codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, rsrc.getPath());
codeOwners.addAll(filterCodeOwners(rsrc, pathCodeOwners.codeOwners()));
- if (pathCodeOwners.ownedByAllUsers()) {
- ownedByAllUsers.set(true);
- fillUpWithRandomUsers(rsrc, codeOwners, limit);
-
- if (codeOwners.size() < limit) {
- logger.atFine().log(
- "tried to fill up the suggestion list with random users,"
- + " but didn't find enough visible accounts"
- + " (wanted number of suggestions = %d, got = %d",
- limit, codeOwners.size());
- }
-
- return true;
- }
-
int distance =
codeOwnerConfig.key().branchNameKey().branch().equals(RefNames.REFS_CONFIG)
? defaultOwnersDistance
@@ -203,6 +198,27 @@
.forEach(
localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance));
+ if (pathCodeOwners.ownedByAllUsers()) {
+ ownedByAllUsers.set(true);
+ ImmutableSet<CodeOwner> addedCodeOwners =
+ fillUpWithRandomUsers(rsrc, codeOwners, limit);
+ addedCodeOwners.forEach(
+ localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance));
+
+ if (codeOwners.size() < limit) {
+ logger.atFine().log(
+ "tried to fill up the suggestion list with random users,"
+ + " but didn't find enough visible accounts"
+ + " (wanted number of suggestions = %d, got = %d",
+ limit, codeOwners.size());
+ }
+ }
+
+ // We always need to iterate over all relevant OWNERS files (even if the limit has already
+ // been reached).
+ // This is needed to collect distance scores for code owners that are mentioned in the
+ // more distant OWNERS files. Those become relevant if further scores are applied later
+ // (e.g. the score for current reviewers of the change).
return true;
});
@@ -216,23 +232,46 @@
if (globalCodeOwners.ownedByAllUsers()) {
ownedByAllUsers.set(true);
- fillUpWithRandomUsers(rsrc, codeOwners, limit);
+ ImmutableSet<CodeOwner> addedCodeOwners = fillUpWithRandomUsers(rsrc, codeOwners, limit);
+ addedCodeOwners.forEach(
+ codeOwner -> distanceScoring.putValueForCodeOwner(codeOwner, globalOwnersDistance));
+ }
+ }
+
+ ImmutableSet<CodeOwner> immutableCodeOwners = ImmutableSet.copyOf(codeOwners);
+ CodeOwnerScorings codeOwnerScorings =
+ createScorings(rsrc, immutableCodeOwners, distanceScoring.build());
+ ImmutableMap<CodeOwner, Double> scoredCodeOwners =
+ codeOwnerScorings.getScorings(immutableCodeOwners);
+
+ ImmutableList<CodeOwner> sortedAndLimitedCodeOwners = sortAndLimit(rsrc, scoredCodeOwners);
+
+ if (highestScoreOnly) {
+ Optional<Double> highestScore =
+ scoredCodeOwners.values().stream().max(Comparator.naturalOrder());
+ if (highestScore.isPresent()) {
+ sortedAndLimitedCodeOwners =
+ sortedAndLimitedCodeOwners.stream()
+ .filter(codeOwner -> scoredCodeOwners.get(codeOwner).equals(highestScore.get()))
+ .collect(toImmutableList());
}
}
CodeOwnersInfo codeOwnersInfo = new CodeOwnersInfo();
codeOwnersInfo.codeOwners =
- codeOwnerJsonFactory
- .create(getFillOptions())
- .format(
- sortAndLimit(
- rsrc,
- CodeOwnerScorings.create(distanceScoring.build()),
- ImmutableSet.copyOf(codeOwners)));
+ codeOwnerJsonFactory.create(getFillOptions()).format(sortedAndLimitedCodeOwners);
codeOwnersInfo.ownedByAllUsers = ownedByAllUsers.get() ? true : null;
return Response.ok(codeOwnersInfo);
}
+ private CodeOwnerScorings createScorings(
+ R rsrc, ImmutableSet<CodeOwner> codeOwners, CodeOwnerScoring distanceScoring) {
+ ImmutableSet.Builder<CodeOwnerScoring> codeOwnerScorings = ImmutableSet.builder();
+ codeOwnerScorings.add(distanceScoring);
+ codeOwnerScorings.addAll(getCodeOwnerScorings(rsrc, codeOwners));
+ return CodeOwnerScorings.create(codeOwnerScorings.build());
+ }
+
private CodeOwnerResolverResult getGlobalCodeOwners(Project.NameKey projectName) {
CodeOwnerResolverResult globalCodeOwners =
codeOwnerResolver
@@ -244,6 +283,19 @@
}
/**
+ * Get further code owner scorings.
+ *
+ * <p>To be overridden by subclasses to include further scorings.
+ *
+ * @param rsrc resource on which the request is being performed
+ * @param codeOwners the code owners
+ */
+ protected ImmutableSet<CodeOwnerScoring> getCodeOwnerScorings(
+ R rsrc, ImmutableSet<CodeOwner> codeOwners) {
+ return ImmutableSet.of();
+ }
+
+ /**
* Filters out code owners that should not be suggested.
*
* <p>The following code owners are filtered out:
@@ -349,26 +401,26 @@
}
private ImmutableList<CodeOwner> sortAndLimit(
- R rsrc, CodeOwnerScorings scorings, ImmutableSet<CodeOwner> codeOwners) {
- return sortCodeOwners(rsrc, seed, scorings, codeOwners).limit(limit).collect(toImmutableList());
+ R rsrc, ImmutableMap<CodeOwner, Double> scoredCodeOwners) {
+ return sortCodeOwners(rsrc, seed, scoredCodeOwners).limit(limit).collect(toImmutableList());
}
/**
* Sorts the code owners.
*
- * <p>Code owners with higher distance score are returned first.
+ * <p>Code owners with higher score are returned first.
*
- * <p>The order of code owners with the same distance score is random.
+ * <p>The order of code owners with the same score is random.
*
* @param rsrc resource on which this REST endpoint is invoked
* @param seed seed that should be used to randomize the order
- * @param scorings the scorings for the code owners
- * @param codeOwners the code owners that should be sorted
+ * @param scoredCodeOwners the code owners with their scores
* @return the sorted code owners
*/
- protected Stream<CodeOwner> sortCodeOwners(
- R rsrc, Optional<Long> seed, CodeOwnerScorings scorings, ImmutableSet<CodeOwner> codeOwners) {
- return randomizeOrder(seed, codeOwners).sorted(scorings.comparingByScorings());
+ private Stream<CodeOwner> sortCodeOwners(
+ R rsrc, Optional<Long> seed, ImmutableMap<CodeOwner, Double> scoredCodeOwners) {
+ return randomizeOrder(seed, scoredCodeOwners.keySet())
+ .sorted(Comparator.comparingDouble(scoredCodeOwners::get).reversed());
}
/**
@@ -393,16 +445,19 @@
* all user.
*
* <p>No-op if code ownership for all users should not be resolved.
+ *
+ * @return the added code owners
*/
- private void fillUpWithRandomUsers(R rsrc, Set<CodeOwner> codeOwners, int limit) {
+ private ImmutableSet<CodeOwner> fillUpWithRandomUsers(
+ R rsrc, Set<CodeOwner> codeOwners, int limit) {
if (!resolveAllUsers || codeOwners.size() >= limit) {
// code ownership for all users should not be resolved or the limit has already been reached
// so that we don't need to add further suggestions
- return;
+ return ImmutableSet.of();
}
logger.atFine().log("filling up with random users");
- codeOwners.addAll(
+ ImmutableSet<CodeOwner> codeOwnersToAdd =
filterCodeOwners(
rsrc,
// ask for 2 times the number of users that we need so that we still have enough
@@ -414,7 +469,9 @@
.stream()
.filter(codeOwner -> !codeOwners.contains(codeOwner))
.limit(limit - codeOwners.size())
- .collect(toImmutableSet()));
+ .collect(toImmutableSet());
+ codeOwners.addAll(codeOwnersToAdd);
+ return codeOwnersToAdd;
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index f475550..e6a2f0d 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -29,7 +29,6 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScoring;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScorings;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.Accounts;
@@ -97,11 +96,8 @@
* only applies if code owners are suggested on changes.
*/
@Override
- protected Stream<CodeOwner> sortCodeOwners(
- CodeOwnersInChangeCollection.PathResource rsrc,
- Optional<Long> seed,
- CodeOwnerScorings scorings,
- ImmutableSet<CodeOwner> codeOwners) {
+ protected ImmutableSet<CodeOwnerScoring> getCodeOwnerScorings(
+ CodeOwnersInChangeCollection.PathResource rsrc, ImmutableSet<CodeOwner> codeOwners) {
// Add scorings for IS_REVIEWER score.
ImmutableSet<Account.Id> reviewers =
rsrc.getRevisionResource()
@@ -116,9 +112,8 @@
reviewers.contains(codeOwner.accountId())
? IS_REVIEWER_SCORING_VALUE
: NO_REVIEWER_SCORING_VALUE));
- scorings = CodeOwnerScorings.appendScoring(scorings, isReviewerScoring.build());
- return super.sortCodeOwners(rsrc, seed, scorings, codeOwners);
+ return ImmutableSet.of(isReviewerScoring.build());
}
@Override
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
index 0430f94..1f2298b 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
@@ -16,6 +16,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Account;
@@ -41,11 +42,33 @@
* /changes/<change-id>/revisions/<revision-id>/owned_paths} requests.
*/
public class GetOwnedPaths implements RestReadView<RevisionResource> {
+ @VisibleForTesting public static final int DEFAULT_LIMIT = 50;
+
private final AccountResolver accountResolver;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
+ private int start;
+ private int limit = DEFAULT_LIMIT;
private String user;
+ @Option(
+ name = "--limit",
+ aliases = {"-n"},
+ metaVar = "CNT",
+ usage = "maximum number of owned path to return (default = " + DEFAULT_LIMIT + ")")
+ 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;
+ }
+
@Option(name = "--user", usage = "user for which the owned paths should be returned")
public void setUser(String user) {
this.user = user;
@@ -62,14 +85,22 @@
public Response<OwnedPathsInfo> apply(RevisionResource revisionResource)
throws BadRequestException, ResourceConflictException, UnresolvableAccountException,
ConfigInvalidException, IOException {
+ validateStartAndLimit();
+
Account.Id accountId = resolveAccount();
ImmutableList<Path> ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- revisionResource.getNotes(), revisionResource.getPatchSet(), accountId, /* limit= */ 0);
+ revisionResource.getNotes(),
+ revisionResource.getPatchSet(),
+ accountId,
+ start,
+ limit + 1);
OwnedPathsInfo ownedPathsInfo = new OwnedPathsInfo();
- ownedPathsInfo.ownedPaths = ownedPaths.stream().map(Path::toString).collect(toImmutableList());
+ ownedPathsInfo.more = ownedPaths.size() > limit ? true : null;
+ ownedPathsInfo.ownedPaths =
+ ownedPaths.stream().limit(limit).map(Path::toString).collect(toImmutableList());
return Response.ok(ownedPathsInfo);
}
@@ -82,4 +113,13 @@
return accountResolver.resolve(user).asUnique().account().id();
}
+
+ private void validateStartAndLimit() throws BadRequestException {
+ if (start < 0) {
+ throw new BadRequestException("start cannot be negative");
+ }
+ if (limit <= 0) {
+ throw new BadRequestException("limit must be positive");
+ }
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/BUILD b/java/com/google/gerrit/plugins/codeowners/testing/BUILD
index 16af5f4..202e994 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/BUILD
+++ b/java/com/google/gerrit/plugins/codeowners/testing/BUILD
@@ -13,6 +13,7 @@
"//java/com/google/gerrit/extensions:api",
"//java/com/google/gerrit/truth",
"//lib:guava",
+ "//lib:jgit",
"//lib/truth",
"//lib/truth:truth-java8-extension",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/api",
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/FileCodeOwnerStatusInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/FileCodeOwnerStatusInfoSubject.java
index 26d3483..5f885e7 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/FileCodeOwnerStatusInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/FileCodeOwnerStatusInfoSubject.java
@@ -17,17 +17,72 @@
import static com.google.common.truth.Truth.assertAbout;
import static com.google.gerrit.plugins.codeowners.testing.PathCodeOwnerStatusInfoSubject.pathCodeOwnerStatusInfos;
import static com.google.gerrit.truth.OptionalSubject.optionals;
+import static java.util.Objects.requireNonNull;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
import com.google.common.truth.ComparableSubject;
+import com.google.common.truth.Correspondence;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.Subject;
import com.google.gerrit.extensions.common.ChangeType;
import com.google.gerrit.plugins.codeowners.api.FileCodeOwnerStatusInfo;
+import com.google.gerrit.plugins.codeowners.api.PathCodeOwnerStatusInfo;
+import com.google.gerrit.plugins.codeowners.backend.FileCodeOwnerStatus;
+import com.google.gerrit.plugins.codeowners.backend.PathCodeOwnerStatus;
+import com.google.gerrit.plugins.codeowners.common.ChangedFile;
+import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.gerrit.truth.OptionalSubject;
import java.util.Optional;
+import org.eclipse.jgit.diff.DiffEntry;
/** {@link Subject} for doing assertions on {@link FileCodeOwnerStatusInfo}s. */
public class FileCodeOwnerStatusInfoSubject extends Subject {
+ private static final ImmutableMap<ChangeType, DiffEntry.ChangeType> CHANGE_TYPE =
+ Maps.immutableEnumMap(
+ new ImmutableMap.Builder<ChangeType, DiffEntry.ChangeType>()
+ .put(ChangeType.ADDED, DiffEntry.ChangeType.ADD)
+ .put(ChangeType.MODIFIED, DiffEntry.ChangeType.MODIFY)
+ .put(ChangeType.DELETED, DiffEntry.ChangeType.DELETE)
+ .put(ChangeType.RENAMED, DiffEntry.ChangeType.RENAME)
+ .put(ChangeType.COPIED, DiffEntry.ChangeType.COPY)
+ .build());
+
+ /**
+ * {@link Correspondence} that maps {@link FileCodeOwnerStatusInfo}s to {@link
+ * FileCodeOwnerStatus}s.
+ */
+ public static final Correspondence<FileCodeOwnerStatusInfo, FileCodeOwnerStatus>
+ isFileCodeOwnerStatus() {
+ return NullAwareCorrespondence.transforming(
+ FileCodeOwnerStatusInfoSubject::toFileCodeOwnerStatus, "is file code owner status");
+ }
+
+ private static FileCodeOwnerStatus toFileCodeOwnerStatus(
+ FileCodeOwnerStatusInfo fileCodeOwnerStatusInfo) {
+ requireNonNull(fileCodeOwnerStatusInfo, "fileCodeOwnerStatusInfo");
+
+ ChangedFile changedFile =
+ ChangedFile.create(
+ Optional.ofNullable(fileCodeOwnerStatusInfo.newPathStatus)
+ .map(pathCodeOwnerStatusInfo -> pathCodeOwnerStatusInfo.path),
+ Optional.ofNullable(fileCodeOwnerStatusInfo.oldPathStatus)
+ .map(pathCodeOwnerStatusInfo -> pathCodeOwnerStatusInfo.path),
+ CHANGE_TYPE.get(fileCodeOwnerStatusInfo.changeType));
+ return FileCodeOwnerStatus.create(
+ changedFile,
+ Optional.ofNullable(fileCodeOwnerStatusInfo.newPathStatus)
+ .map(FileCodeOwnerStatusInfoSubject::toPathCodeOwnerStatus),
+ Optional.ofNullable(fileCodeOwnerStatusInfo.oldPathStatus)
+ .map(FileCodeOwnerStatusInfoSubject::toPathCodeOwnerStatus));
+ }
+
+ private static PathCodeOwnerStatus toPathCodeOwnerStatus(
+ PathCodeOwnerStatusInfo pathCodeOwnerStatusInfo) {
+ requireNonNull(pathCodeOwnerStatusInfo, "pathCodeOwnerStatusInfo");
+ return PathCodeOwnerStatus.create(pathCodeOwnerStatusInfo.path, pathCodeOwnerStatusInfo.status);
+ }
+
/**
* Starts fluent chain to do assertions on a {@link FileCodeOwnerStatusInfo}.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java
index df6b1d0..d6a1072 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertAbout;
+import com.google.common.truth.BooleanSubject;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.IterableSubject;
import com.google.common.truth.Subject;
@@ -48,6 +49,10 @@
return check("ownedPaths()").that(ownedPathsInfo().ownedPaths);
}
+ public BooleanSubject hasMoreThat() {
+ return check("more()").that(ownedPathsInfo().more);
+ }
+
private OwnedPathsInfo ownedPathsInfo() {
isNotNull();
return ownedPathsInfo;
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
index 743bef0..b2d421d 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -114,12 +114,18 @@
@Test
public void getCodeOwnersForAbsolutePath() throws Exception {
- testGetCodeOwners(true);
+ testGetCodeOwners(/* useAbsolutePath= */ true);
}
@Test
public void getCodeOwnersForNonAbsolutePath() throws Exception {
- testGetCodeOwners(false);
+ testGetCodeOwners(/* useAbsolutePath= */ false);
+ }
+
+ @Test
+ public void getCodeOwnersForAnonymousUser() throws Exception {
+ requestScopeOperations.setApiUserAnonymous();
+ testGetCodeOwners(/* useAbsolutePath= */ true);
}
private void testGetCodeOwners(boolean useAbsolutePath) throws Exception {
@@ -1385,4 +1391,77 @@
.inOrder();
assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue();
}
+
+ @Test
+ public void getCodeOwnersWithHighestScoreOnly() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // create some code owner configs
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/bar/")
+ .addCodeOwnerEmail(user.email())
+ .addCodeOwnerEmail(user2.email())
+ .create();
+
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().withHighestScoreOnly(/* highestScoreOnly= */ false),
+ "/foo/bar/baz.md");
+ assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(3);
+ // the first 2 code owners have the same scoring, so their order is random and we don't know
+ // which of them we get get first and second
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .element(0)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id());
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .element(1)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id());
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .element(2)
+ .hasAccountIdThat()
+ .isEqualTo(admin.id());
+
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().withHighestScoreOnly(/* highestScoreOnly= */ true),
+ "/foo/bar/baz.md");
+ assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(2);
+ // the first 2 code owners have the same scoring, so their order is random and we don't know
+ // which of them we get get first and second
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .element(0)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id());
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .element(1)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id());
+ }
+
+ @Test
+ public void getCodeOwnersWithHighestScoreOnly_noCodeOwners() throws Exception {
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().withHighestScoreOnly(/* highestScoreOnly= */ true),
+ "/foo/bar/baz.md");
+ assertThat(codeOwnersInfo).hasCodeOwnersThat().isEmpty();
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
index 00577c7..cf6e515 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
@@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestMetricMaker;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.Nullable;
@@ -50,6 +51,7 @@
/** Acceptance test for {@code com.google.gerrit.plugins.codeowners.backend.CodeOwnerSubmitRule}. */
public class CodeOwnerSubmitRuleIT extends AbstractCodeOwnersIT {
@Inject private ProjectOperations projectOperations;
+ @Inject private TestMetricMaker testMetricMaker;
@Test
public void changeIsSubmittableIfCodeOwnersFuctionalityIsDisabled() throws Exception {
@@ -103,7 +105,7 @@
// Verify that the code owner status for the changed file is INSUFFICIENT_REVIEWERS.
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.onlyElement()
@@ -156,7 +158,7 @@
// Verify that the code owner status for the changed file is PENDING.
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.onlyElement()
@@ -206,7 +208,7 @@
// Verify that the code owner status for the changed file is APPROVED.
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.onlyElement()
@@ -247,7 +249,7 @@
// Verify that the code owner status for the changed file is APPROVED.
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.onlyElement()
@@ -496,6 +498,37 @@
assertThat(gApi.changes().id(changeId).get().status).isEqualTo(ChangeStatus.MERGED);
}
+ @Test
+ public void submitRuleIsInvokedOnlyOnceWhenGettingChangeDetails() throws Exception {
+ PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content");
+ String changeId = r.getChangeId();
+
+ testMetricMaker.reset();
+ gApi.changes()
+ .id(changeId)
+ .get(ListChangesOption.ALL_REVISIONS, ListChangesOption.CURRENT_ACTIONS);
+
+ // Submit rules are computed freshly, but only once.
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_submit_rule_runs"))
+ .isEqualTo(1);
+ }
+
+ @Test
+ public void submitRuleIsNotInvokedWhenQueryingChange() throws Exception {
+ PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content");
+ String changeId = r.getChangeId();
+
+ testMetricMaker.reset();
+ gApi.changes()
+ .query(changeId)
+ .withOptions(ListChangesOption.ALL_REVISIONS, ListChangesOption.CURRENT_ACTIONS)
+ .get();
+
+ // Submit rule evaluation results from the change index are reused
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_submit_rule_runs"))
+ .isEqualTo(0);
+ }
+
private void deleteAutoMergeBranch(ObjectId mergeCommit) throws Exception {
try (Repository repo = repoManager.openRepository(project)) {
RefUpdate ru = repo.updateRef(RefNames.refsCacheAutomerge(mergeCommit.name()));
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 531f118..79f235f 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
@@ -15,15 +15,15 @@
package com.google.gerrit.plugins.codeowners.acceptance.api;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerStatusInfoSubject.assertThat;
+import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusInfoSubject.isFileCodeOwnerStatus;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
-import com.google.gerrit.extensions.common.ChangeType;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatusInfo;
+import com.google.gerrit.plugins.codeowners.backend.FileCodeOwnerStatus;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
-import com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusInfoSubject;
import com.google.inject.Inject;
import org.junit.Test;
@@ -42,13 +42,7 @@
public void getStatus() throws Exception {
TestAccount user2 = accountCreator.user2();
- codeOwnerConfigOperations
- .newCodeOwnerConfig()
- .project(project)
- .branch("master")
- .folderPath("/foo/")
- .addCodeOwnerEmail(user.email())
- .create();
+ setAsCodeOwners("/foo/", user);
String path = "foo/bar.baz";
PushOneCommit.Result r = createChange("Change Adding A File", path, "file content");
@@ -62,113 +56,58 @@
recommend(changeId);
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasPatchSetNumberThat()
.isEqualTo(r.getChange().currentPatchSet().id().get());
- FileCodeOwnerStatusInfoSubject fileCodeOwnerStatusInfoSubject =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().onlyElement();
- fileCodeOwnerStatusInfoSubject.hasChangeTypeThat().isEqualTo(ChangeType.ADDED);
- fileCodeOwnerStatusInfoSubject.hasNewPathStatusThat().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusInfoSubject
- .hasNewPathStatusThat()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
+
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
}
@Test
public void getStatusForRenamedFile() throws Exception {
TestAccount user2 = accountCreator.user2();
- codeOwnerConfigOperations
- .newCodeOwnerConfig()
- .project(project)
- .branch("master")
- .folderPath("/foo/bar")
- .addCodeOwnerEmail(user.email())
- .create();
- codeOwnerConfigOperations
- .newCodeOwnerConfig()
- .project(project)
- .branch("master")
- .folderPath("/foo/baz")
- .addCodeOwnerEmail(user2.email())
- .create();
+ setAsCodeOwners("/foo/bar/", user);
+ setAsCodeOwners("/foo/baz/", user2);
String oldPath = "foo/bar/abc.txt";
String newPath = "foo/baz/abc.txt";
String changeId = createChangeWithFileRename(oldPath, newPath);
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().hasSize(2);
- FileCodeOwnerStatusInfoSubject fileCodeOwnerStatusInfoSubject1 =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(0);
- fileCodeOwnerStatusInfoSubject1.hasChangeTypeThat().isEqualTo(ChangeType.DELETED);
- fileCodeOwnerStatusInfoSubject1.hasOldPathStatusThat().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusInfoSubject1
- .hasOldPathStatusThat()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- FileCodeOwnerStatusInfoSubject fileCodeOwnerStatusInfoSubject2 =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(1);
- fileCodeOwnerStatusInfoSubject2.hasChangeTypeThat().isEqualTo(ChangeType.ADDED);
- fileCodeOwnerStatusInfoSubject2.hasNewPathStatusThat().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusInfoSubject2
- .hasNewPathStatusThat()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
+ 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();
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().hasSize(2);
- fileCodeOwnerStatusInfoSubject1 =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(0);
- fileCodeOwnerStatusInfoSubject1.hasChangeTypeThat().isEqualTo(ChangeType.DELETED);
- fileCodeOwnerStatusInfoSubject1.hasOldPathStatusThat().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusInfoSubject1
- .hasOldPathStatusThat()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
- fileCodeOwnerStatusInfoSubject2 =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(1);
- fileCodeOwnerStatusInfoSubject2.hasChangeTypeThat().isEqualTo(ChangeType.ADDED);
- fileCodeOwnerStatusInfoSubject2.hasNewPathStatusThat().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusInfoSubject2
- .hasNewPathStatusThat()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
+ 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();
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().hasSize(2);
- fileCodeOwnerStatusInfoSubject1 =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(0);
- fileCodeOwnerStatusInfoSubject1.hasChangeTypeThat().isEqualTo(ChangeType.DELETED);
- fileCodeOwnerStatusInfoSubject1.hasOldPathStatusThat().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusInfoSubject1
- .hasOldPathStatusThat()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
- fileCodeOwnerStatusInfoSubject2 =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(1);
- fileCodeOwnerStatusInfoSubject2.hasChangeTypeThat().isEqualTo(ChangeType.ADDED);
- fileCodeOwnerStatusInfoSubject2.hasNewPathStatusThat().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusInfoSubject2
- .hasNewPathStatusThat()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
+ codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.PENDING));
}
@Test
@@ -177,15 +116,11 @@
String path = "foo/bar.baz";
String changeId = createChange("Change Adding A File", path, "file content").getChangeId();
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
- FileCodeOwnerStatusInfoSubject fileCodeOwnerStatusInfoSubject =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().onlyElement();
- fileCodeOwnerStatusInfoSubject.hasChangeTypeThat().isEqualTo(ChangeType.ADDED);
- fileCodeOwnerStatusInfoSubject.hasNewPathStatusThat().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusInfoSubject
- .hasNewPathStatusThat()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
index e0802a1..1feefc6 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -36,6 +36,7 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.api.CodeOwners;
import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.inject.Inject;
import java.util.List;
@@ -389,4 +390,43 @@
.containsExactly(user2.id(), user.id(), admin.id())
.inOrder();
}
+
+ @Test
+ public void codeOwnersSortedByDistance_fileOwnedByAllUsers() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(CodeOwnerResolver.ALL_USERS_WILDCARD)
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/bar/")
+ .addCodeOwnerEmail(user2.email())
+ .create();
+
+ // None of the code owners is a reviewer, hence the sorting is done only by distance.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user2.id(), user.id(), admin.id())
+ .inOrder();
+ assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue();
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetOwnedPathsIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetOwnedPathsIT.java
index 6040196..681513b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetOwnedPathsIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetOwnedPathsIT.java
@@ -26,6 +26,7 @@
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.api.OwnedPathsInfo;
+import com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.inject.Inject;
import org.junit.Test;
@@ -33,6 +34,10 @@
/**
* Acceptance test for the {@link com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths} REST
* endpoint.
+ *
+ * <p>Further tests for the {@link com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths} REST
+ * endpoint that require using the REST API are implemented in {@link
+ * com.google.gerrit.plugins.codeowners.acceptance.restapi.GetOwnedPathRestIT}.
*/
public class GetOwnedPathsIT extends AbstractCodeOwnersIT {
@Inject private RequestScopeOperations requestScopeOperations;
@@ -126,6 +131,7 @@
.forUser(user.email())
.get();
assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path1, path2).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isNull();
}
@Test
@@ -179,4 +185,260 @@
.get();
assertThat(ownedPathsInfo).hasOwnedPathsThat().isEmpty();
}
+
+ @Test
+ public void getOwnedPathsWithStart() throws Exception {
+ setAsRootCodeOwners(user);
+
+ String path1 = "/bar/baz.md";
+ String path2 = "/bar/foo.md";
+ String path3 = "/foo/bar/baz.md";
+ String path4 = "/foo/baz/bar.md";
+
+ String changeId =
+ createChange(
+ "test change",
+ 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"))
+ .getChangeId();
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(0)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo)
+ .hasOwnedPathsThat()
+ .containsExactly(path1, path2, path3, path4)
+ .inOrder();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(1)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path2, path3, path4).inOrder();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(2)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path3, path4).inOrder();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(3)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path4);
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(4)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().isEmpty();
+ }
+
+ @Test
+ public void getOwnedPathsWithLimit() throws Exception {
+ setAsRootCodeOwners(user);
+
+ String path1 = "/bar/baz.md";
+ String path2 = "/bar/foo.md";
+ String path3 = "/foo/bar/baz.md";
+ String path4 = "/foo/baz/bar.md";
+
+ String changeId =
+ createChange(
+ "test change",
+ 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"))
+ .getChangeId();
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(1)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path1);
+ assertThat(ownedPathsInfo).hasMoreThat().isTrue();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(2)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path1, path2).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isTrue();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(3)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path1, path2, path3).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isTrue();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(4)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo)
+ .hasOwnedPathsThat()
+ .containsExactly(path1, path2, path3, path4)
+ .inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isNull();
+ }
+
+ @Test
+ public void getOwnedPathsWithStartAndLimit() throws Exception {
+ setAsRootCodeOwners(user);
+
+ String path1 = "/bar/baz.md";
+ String path2 = "/bar/foo.md";
+ String path3 = "/foo/bar/baz.md";
+ String path4 = "/foo/baz/bar.md";
+
+ String changeId =
+ createChange(
+ "test change",
+ 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"))
+ .getChangeId();
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(1)
+ .withLimit(2)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path2, path3).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isTrue();
+ }
+
+ @Test
+ public void getOwnedPathsLimitedByDefault() throws Exception {
+ setAsRootCodeOwners(user);
+
+ ImmutableMap.Builder<String, String> files = ImmutableMap.builder();
+ for (int i = 1; i <= GetOwnedPaths.DEFAULT_LIMIT + 1; i++) {
+ files.put(String.format("foo-%d.txt", i), "file content");
+ }
+
+ String changeId = createChange("test change", files.build()).getChangeId();
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().hasSize(GetOwnedPaths.DEFAULT_LIMIT);
+ }
+
+ @Test
+ public void startCannotBeNegative() throws Exception {
+ String changeId = createChange().getChangeId();
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(-1)
+ .forUser(user.email())
+ .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)
+ .current()
+ .getOwnedPaths()
+ .withLimit(-1)
+ .forUser(user.email())
+ .get());
+ assertThat(exception).hasMessageThat().isEqualTo("limit must be positive");
+ }
+
+ @Test
+ public void cannotGetOwnedPathWithoutLimit() throws Exception {
+ String changeId = createChange().getChangeId();
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(0)
+ .forUser(user.email())
+ .get());
+ assertThat(exception).hasMessageThat().isEqualTo("limit must be positive");
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetOwnedPathRestIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetOwnedPathRestIT.java
new file mode 100644
index 0000000..e5287df
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetOwnedPathRestIT.java
@@ -0,0 +1,72 @@
+// 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.acceptance.restapi;
+
+import static com.google.common.truth.Truth.assertThat;
+import static java.util.stream.Collectors.joining;
+
+import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
+import java.util.Arrays;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Acceptance test for the {@link com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths} REST
+ * endpoint. that require using via REST.
+ *
+ * <p>Acceptance test for the {@link com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths}
+ * REST endpoint that can use the Java API are implemented in {@link
+ * com.google.gerrit.plugins.codeowners.acceptance.api.GetOwnedPathsIT}.
+ */
+public class GetOwnedPathRestIT extends AbstractCodeOwnersIT {
+ private String changeId;
+
+ @Before
+ public void createTestChange() throws Exception {
+ changeId = createChange().getChangeId();
+ }
+
+ @Test
+ public void cannotGetOwnedPathsWithInvalidStart() throws Exception {
+ RestResponse r = adminRestSession.get(getUrl("user=" + user.email(), "start=invalid"));
+ r.assertBadRequest();
+ assertThat(r.getEntityContent()).contains("\"invalid\" is not a valid value for \"--start\"");
+ }
+
+ @Test
+ public void cannotGetOwnedPathsWithInvalidLimit() throws Exception {
+ RestResponse r = adminRestSession.get(getUrl("user=" + user.email(), "limit=invalid"));
+ r.assertBadRequest();
+ assertThat(r.getEntityContent()).contains("\"invalid\" is not a valid value for \"--limit\"");
+ }
+
+ private String getUrl(String... parameters) {
+ StringBuilder b = new StringBuilder();
+ b.append(getUrl());
+ String paramaterString = Arrays.stream(parameters).collect(joining("&"));
+ if (!paramaterString.isEmpty()) {
+ b.append('?').append(paramaterString);
+ }
+ return b.toString();
+ }
+
+ private String getUrl() {
+ return String.format(
+ "/changes/%s/revisions/%s/owned_paths",
+ IdString.fromDecoded(changeId), IdString.fromDecoded("current"));
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
index 6abb86e..a1ac734 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
@@ -44,14 +44,14 @@
@Test
public void getNewPath_diffEntry() throws Exception {
String newPath = "foo/bar/baz.txt";
- setupDiffEntry(newPath, null, ChangeType.ADD);
+ setupDiffEntry(newPath, /* oldPath= */ null, ChangeType.ADD);
assertThat(ChangedFile.create(diffEntry).newPath()).value().isEqualTo(Paths.get("/" + newPath));
}
@Test
public void getNewPath_patchListEntry() throws Exception {
String newPath = "foo/bar/baz.txt";
- setupPatchListEntry(newPath, null, Patch.ChangeType.ADDED);
+ setupPatchListEntry(newPath, /* oldPath= */ null, Patch.ChangeType.ADDED);
assertThat(ChangedFile.create(patchListEntry).newPath())
.value()
.isEqualTo(Paths.get("/" + newPath));
@@ -59,20 +59,20 @@
@Test
public void getNewPathWhenNewPathIsNotSet_diffEntry() throws Exception {
- setupDiffEntry(null, null, ChangeType.ADD);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
assertThat(ChangedFile.create(diffEntry).newPath()).isEmpty();
}
@Test
public void getNewPathWhenNewPathIsNotSet_patchListEntry() throws Exception {
- setupPatchListEntry(null, null, Patch.ChangeType.ADDED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
assertThat(ChangedFile.create(patchListEntry).newPath()).isEmpty();
}
@Test
public void hasNewPath_diffEntry() throws Exception {
String newPath = "foo/bar/baz.txt";
- setupDiffEntry(newPath, null, ChangeType.ADD);
+ setupDiffEntry(newPath, /* oldPath= */ null, ChangeType.ADD);
ChangedFile changedFile = ChangedFile.create(diffEntry);
assertThat(changedFile.hasNewPath(Paths.get("/" + newPath))).isTrue();
@@ -82,7 +82,7 @@
@Test
public void hasNewPath_patchListEntry() throws Exception {
String newPath = "foo/bar/baz.txt";
- setupPatchListEntry(newPath, null, Patch.ChangeType.ADDED);
+ setupPatchListEntry(newPath, /* oldPath= */ null, Patch.ChangeType.ADDED);
ChangedFile changedFile = ChangedFile.create(patchListEntry);
assertThat(changedFile.hasNewPath(Paths.get("/" + newPath))).isTrue();
@@ -91,28 +91,30 @@
@Test
public void cannotCheckHasNewPathWithNull_diffEntry() throws Exception {
- setupDiffEntry(null, null, ChangeType.ADD);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> ChangedFile.create(diffEntry).hasNewPath(null));
+ NullPointerException.class,
+ () -> ChangedFile.create(diffEntry).hasNewPath(/* absolutePath= */ null));
assertThat(npe).hasMessageThat().isEqualTo("absolutePath");
}
@Test
public void cannotCheckHasNewPathWithNull_patchListEntry() throws Exception {
- setupPatchListEntry(null, null, Patch.ChangeType.ADDED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> ChangedFile.create(patchListEntry).hasNewPath(null));
+ NullPointerException.class,
+ () -> ChangedFile.create(patchListEntry).hasNewPath(/* absolutePath= */ null));
assertThat(npe).hasMessageThat().isEqualTo("absolutePath");
}
@Test
public void cannotCheckHasNewPathWithRelativePath_diffEntry() throws Exception {
String relativePath = "foo/bar/baz.txt";
- setupDiffEntry(null, null, ChangeType.ADD);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
IllegalStateException exception =
assertThrows(
IllegalStateException.class,
@@ -125,7 +127,7 @@
@Test
public void cannotCheckHasNewPathWithRelativePath_patchListEntry() throws Exception {
String relativePath = "foo/bar/baz.txt";
- setupPatchListEntry(null, null, Patch.ChangeType.ADDED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
IllegalStateException exception =
assertThrows(
IllegalStateException.class,
@@ -138,14 +140,14 @@
@Test
public void getOldPath_diffEntry() throws Exception {
String oldPath = "foo/bar/baz.txt";
- setupDiffEntry(null, oldPath, ChangeType.DELETE);
+ setupDiffEntry(/* newPath= */ null, oldPath, ChangeType.DELETE);
assertThat(ChangedFile.create(diffEntry).oldPath()).value().isEqualTo(Paths.get("/" + oldPath));
}
@Test
public void getOldPath_patchListEntry() throws Exception {
String oldPath = "foo/bar/baz.txt";
- setupPatchListEntry(null, oldPath, Patch.ChangeType.DELETED);
+ setupPatchListEntry(/* newPath= */ null, oldPath, Patch.ChangeType.DELETED);
assertThat(ChangedFile.create(patchListEntry).oldPath())
.value()
.isEqualTo(Paths.get("/" + oldPath));
@@ -153,21 +155,21 @@
@Test
public void getOldPathWhenOldPathIsNotSet_diffEntry() throws Exception {
- setupDiffEntry(null, null, ChangeType.DELETE);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
when(diffEntry.getOldPath()).thenReturn(DiffEntry.DEV_NULL);
assertThat(ChangedFile.create(diffEntry).oldPath()).isEmpty();
}
@Test
public void getOldPathWhenOldPathIsNotSet_patchListEntry() throws Exception {
- setupPatchListEntry(null, null, Patch.ChangeType.DELETED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
assertThat(ChangedFile.create(patchListEntry).oldPath()).isEmpty();
}
@Test
public void hasOldPath_diffEntry() throws Exception {
String oldPath = "foo/bar/baz.txt";
- setupDiffEntry(null, oldPath, ChangeType.DELETE);
+ setupDiffEntry(/* newPath= */ null, oldPath, ChangeType.DELETE);
ChangedFile changedFile = ChangedFile.create(diffEntry);
assertThat(changedFile.hasOldPath(Paths.get("/" + oldPath))).isTrue();
@@ -177,7 +179,7 @@
@Test
public void hasOldPath_patchListEntry() throws Exception {
String oldPath = "foo/bar/baz.txt";
- setupPatchListEntry(null, oldPath, Patch.ChangeType.DELETED);
+ setupPatchListEntry(/* newPath= */ null, oldPath, Patch.ChangeType.DELETED);
ChangedFile changedFile = ChangedFile.create(patchListEntry);
assertThat(changedFile.hasOldPath(Paths.get("/" + oldPath))).isTrue();
@@ -186,25 +188,27 @@
@Test
public void cannotCheckHasOldPathWithNull_diffEntry() throws Exception {
- setupDiffEntry(null, null, ChangeType.DELETE);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> ChangedFile.create(diffEntry).hasOldPath(null));
+ NullPointerException.class,
+ () -> ChangedFile.create(diffEntry).hasOldPath(/* absolutePath= */ null));
assertThat(npe).hasMessageThat().isEqualTo("absolutePath");
}
@Test
public void cannotCheckHasOldPathWithNull_patchListEntry() throws Exception {
- setupPatchListEntry(null, null, Patch.ChangeType.DELETED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> ChangedFile.create(patchListEntry).hasOldPath(null));
+ NullPointerException.class,
+ () -> ChangedFile.create(patchListEntry).hasOldPath(/* absolutePath= */ null));
assertThat(npe).hasMessageThat().isEqualTo("absolutePath");
}
@Test
public void cannotCheckHasOldPathWithRelativePath_diffEntry() throws Exception {
- setupDiffEntry(null, null, ChangeType.DELETE);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
String relativePath = "foo/bar/baz.txt";
IllegalStateException exception =
assertThrows(
@@ -217,7 +221,7 @@
@Test
public void cannotCheckHasOldPathWithRelativePath_patchListEntry() throws Exception {
- setupPatchListEntry(null, null, Patch.ChangeType.DELETED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
String relativePath = "foo/bar/baz.txt";
IllegalStateException exception =
assertThrows(
@@ -230,7 +234,7 @@
@Test
public void isRename_diffEntry() throws Exception {
- setupDiffEntry(null, null, ChangeType.RENAME);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.RENAME);
ChangedFile changedFile = ChangedFile.create(diffEntry);
assertThat(changedFile.isRename()).isTrue();
assertThat(changedFile.isDeletion()).isFalse();
@@ -238,7 +242,7 @@
@Test
public void isRename_patchListEntry() throws Exception {
- setupPatchListEntry(null, null, Patch.ChangeType.RENAMED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.RENAMED);
ChangedFile changedFile = ChangedFile.create(patchListEntry);
assertThat(changedFile.isRename()).isTrue();
assertThat(changedFile.isDeletion()).isFalse();
@@ -246,7 +250,7 @@
@Test
public void isDeletion_diffEntry() throws Exception {
- setupDiffEntry(null, null, ChangeType.DELETE);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
ChangedFile changedFile = ChangedFile.create(diffEntry);
assertThat(changedFile.isRename()).isFalse();
assertThat(changedFile.isDeletion()).isTrue();
@@ -254,7 +258,7 @@
@Test
public void isDeletion_patchListEntry() throws Exception {
- setupPatchListEntry(null, null, Patch.ChangeType.DELETED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
ChangedFile changedFile = ChangedFile.create(patchListEntry);
assertThat(changedFile.isRename()).isFalse();
assertThat(changedFile.isDeletion()).isTrue();
@@ -262,7 +266,7 @@
@Test
public void isAddition_diffEntry() throws Exception {
- setupDiffEntry(null, null, ChangeType.ADD);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
ChangedFile changedFile = ChangedFile.create(diffEntry);
assertThat(changedFile.isRename()).isFalse();
assertThat(changedFile.isDeletion()).isFalse();
@@ -270,7 +274,7 @@
@Test
public void isAddition_patchListEntry() throws Exception {
- setupPatchListEntry(null, null, Patch.ChangeType.ADDED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
ChangedFile changedFile = ChangedFile.create(patchListEntry);
assertThat(changedFile.isRename()).isFalse();
assertThat(changedFile.isDeletion()).isFalse();
@@ -278,7 +282,7 @@
@Test
public void isModify_diffEntry() throws Exception {
- setupDiffEntry(null, null, ChangeType.MODIFY);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.MODIFY);
ChangedFile changedFile = ChangedFile.create(diffEntry);
assertThat(changedFile.isRename()).isFalse();
assertThat(changedFile.isDeletion()).isFalse();
@@ -286,7 +290,7 @@
@Test
public void isModify_patchListEntry() throws Exception {
- setupPatchListEntry(null, null, Patch.ChangeType.MODIFIED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.MODIFIED);
ChangedFile changedFile = ChangedFile.create(patchListEntry);
assertThat(changedFile.isRename()).isFalse();
assertThat(changedFile.isDeletion()).isFalse();
@@ -294,7 +298,7 @@
@Test
public void isCopy_diffEntry() throws Exception {
- setupDiffEntry(null, null, ChangeType.COPY);
+ setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.COPY);
ChangedFile changedFile = ChangedFile.create(diffEntry);
assertThat(changedFile.isRename()).isFalse();
assertThat(changedFile.isDeletion()).isFalse();
@@ -302,7 +306,7 @@
@Test
public void isCopy_patchListEntry() throws Exception {
- setupPatchListEntry(null, null, Patch.ChangeType.COPIED);
+ setupPatchListEntry(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.COPIED);
ChangedFile changedFile = ChangedFile.create(patchListEntry);
assertThat(changedFile.isRename()).isFalse();
assertThat(changedFile.isDeletion()).isFalse();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
index 2bbc4f4..8e70676 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -66,7 +66,8 @@
@Test
public void cannotComputeForNullRevisionResource() throws Exception {
NullPointerException npe =
- assertThrows(NullPointerException.class, () -> changedFiles.compute(null));
+ assertThrows(
+ NullPointerException.class, () -> changedFiles.compute(/* revisionResource= */ null));
assertThat(npe).hasMessageThat().isEqualTo("revisionResource");
}
@@ -74,14 +75,16 @@
public void cannotComputeForNullProject() throws Exception {
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> changedFiles.compute(null, ObjectId.zeroId()));
+ NullPointerException.class,
+ () -> changedFiles.compute(/* project= */ null, ObjectId.zeroId()));
assertThat(npe).hasMessageThat().isEqualTo("project");
}
@Test
public void cannotComputeForNullRevision() throws Exception {
NullPointerException npe =
- assertThrows(NullPointerException.class, () -> changedFiles.compute(project, null));
+ assertThrows(
+ NullPointerException.class, () -> changedFiles.compute(project, /* revision= */ null));
assertThat(npe).hasMessageThat().isEqualTo("revision");
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 1023d2c..596eead 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -16,7 +16,6 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
-import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatCollection;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
@@ -24,6 +23,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
@@ -31,8 +31,11 @@
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelDefinitionInput;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
@@ -97,17 +100,9 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -129,17 +124,9 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.modification(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -158,17 +145,9 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -188,27 +167,10 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
- assertThatCollection(fileCodeOwnerStatuses).element(0);
- fileCodeOwnerStatusSubject1.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject1
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject1.hasChangedFile().isNoDeletion();
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
- assertThatCollection(fileCodeOwnerStatuses).element(1);
- fileCodeOwnerStatusSubject2.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject2
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject2.hasChangedFile().isDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -230,17 +192,8 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
- fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
}
@Test
@@ -264,17 +217,8 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
- fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.modification(path, CodeOwnerStatus.PENDING));
}
@Test
@@ -295,17 +239,8 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.deletion(path, CodeOwnerStatus.PENDING));
}
@Test
@@ -327,27 +262,10 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
- assertThatCollection(fileCodeOwnerStatuses).element(0);
- fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject1
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
- fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
- assertThatCollection(fileCodeOwnerStatuses).element(1);
- fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject2
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -369,27 +287,10 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
- assertThatCollection(fileCodeOwnerStatuses).element(0);
- fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject1
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
- assertThatCollection(fileCodeOwnerStatuses).element(1);
- fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject2
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.PENDING));
}
@Test
@@ -406,17 +307,8 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -435,17 +327,8 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.modification(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -461,17 +344,8 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.deletion(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -489,27 +363,10 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
- assertThatCollection(fileCodeOwnerStatuses).element(0);
- fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject1
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
- assertThatCollection(fileCodeOwnerStatuses).element(1);
- fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject2
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.APPROVED),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -527,27 +384,10 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
- assertThatCollection(fileCodeOwnerStatuses).element(0);
- fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject1
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
- assertThatCollection(fileCodeOwnerStatuses).element(1);
- fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject2
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.APPROVED));
}
@Test
@@ -574,20 +414,13 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -617,20 +450,13 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.modification(
+ path,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -656,20 +482,13 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(
+ path,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -698,30 +517,14 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
- assertThatCollection(fileCodeOwnerStatuses).element(0);
- fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject1
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
- assertThatCollection(fileCodeOwnerStatuses).element(1);
- fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject2
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(
+ oldPath,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -750,30 +553,14 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
- assertThatCollection(fileCodeOwnerStatuses).element(0);
- fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject1
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
- assertThatCollection(fileCodeOwnerStatuses).element(1);
- fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject2
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(
+ newPath,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -788,17 +575,9 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -817,17 +596,9 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -851,14 +622,9 @@
// implicit approvals are disabled).
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Add an approval by a user that is a code owner only through the global code ownership.
approve(changeId);
@@ -866,13 +632,8 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -904,17 +665,13 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -937,27 +694,17 @@
// approval by default).
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Add a user as reviewer that is a code owner.
gApi.changes().id(changeId).addReviewer(user.email());
// Check that the status of the file is PENDING now.
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
}
@Test
@@ -976,14 +723,9 @@
// Verify that the file is not approved yet.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Let the bot approve the change.
projectOperations
@@ -997,13 +739,8 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1031,17 +768,13 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -1060,14 +793,9 @@
// Verify that the status of the file is INSUFFICIENT_REVIEWERS.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Add the bot approve as reviewer.
gApi.changes().id(changeId).addReviewer(bot.email());
@@ -1075,13 +803,8 @@
// Check that the status of the file is PENDING now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
// Let the bot approve the change.
projectOperations
@@ -1095,13 +818,8 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1117,14 +835,9 @@
// implicit approvals are disabled).
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Add an approval by a user that is a code owner only through the global code ownership.
approve(changeId);
@@ -1132,13 +845,8 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1165,17 +873,13 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -1190,27 +894,17 @@
// approval by default).
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Add a user as reviewer that is a code owner only through the global code ownership.
gApi.changes().id(changeId).addReviewer(user.email());
// Check that the status of the file is PENDING now.
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
}
@Test
@@ -1239,14 +933,8 @@
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
// The expected status is APPROVED since 'user' which is configured as code owner on the root
// level approved the change.
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1254,6 +942,9 @@
public void getStatus_overrideApprovesAllFiles() throws Exception {
createOwnersOverrideLabel();
+ String path1 = "foo/baz.config";
+ String path2 = "bar/baz.config";
+
// Create a change.
String changeId =
pushFactory
@@ -1262,33 +953,28 @@
testRepo,
"Test Change",
ImmutableMap.of(
- "foo/baz.config", "content",
- "bar/baz.config", "other content"))
+ path1, "content",
+ path2, "other content"))
.to("refs/for/master")
.getChangeId();
// Without Owners-Override approval the expected status is INSUFFICIENT_REVIEWERS.
- for (FileCodeOwnerStatus fileCodeOwnerStatus :
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId))) {
- assertThat(fileCodeOwnerStatus)
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Add an override approval.
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))) {
- assertThat(fileCodeOwnerStatus)
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- }
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.APPROVED),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1299,6 +985,9 @@
createOwnersOverrideLabel();
createOwnersOverrideLabel("Another-Override");
+ String path1 = "foo/baz.config";
+ String path2 = "bar/baz.config";
+
// Create a change.
String changeId =
pushFactory
@@ -1307,59 +996,48 @@
testRepo,
"Test Change",
ImmutableMap.of(
- "foo/baz.config", "content",
- "bar/baz.config", "other content"))
+ path1, "content",
+ path2, "other content"))
.to("refs/for/master")
.getChangeId();
// Without override approval the expected status is INSUFFICIENT_REVIEWERS.
- for (FileCodeOwnerStatus fileCodeOwnerStatus :
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId))) {
- assertThat(fileCodeOwnerStatus)
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Add an override approval.
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))) {
- assertThat(fileCodeOwnerStatus)
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- }
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.APPROVED),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.APPROVED));
// Delete the override approval.
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))) {
- assertThat(fileCodeOwnerStatus)
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Add another override approval.
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))) {
- assertThat(fileCodeOwnerStatus)
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- }
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path1, CodeOwnerStatus.APPROVED),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1510,14 +1188,9 @@
// Verify that the file is not approved yet.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Let the 'user' approve the change.
projectOperations
@@ -1531,13 +1204,8 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
// Change some other file ('user' who uploads the change is a code owner and hence owner
// approvals are implicit for this change)
@@ -1556,13 +1224,8 @@
// Check that the file is still approved.
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1584,14 +1247,9 @@
// Verify that the file is not approved yet.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Apply an override
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
@@ -1599,13 +1257,8 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
// Change some other file and submit the change with an override.
String changeId2 =
@@ -1623,13 +1276,8 @@
// Check that the file is still approved.
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1648,14 +1296,9 @@
// Verify that the file is not approved yet.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Let 'user' approve the change (vote Code-Review+2)
projectOperations
@@ -1669,13 +1312,8 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1710,14 +1348,9 @@
// Verify that the file is not approved yet.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Let 'user2' override with Owners-Override+2
requestScopeOperations.setApiUser(user2.id());
@@ -1726,72 +1359,8 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- }
-
- @Test
- public void projectOwnersAreNoCodeOwnersIfDefaultCodeOwnerConfigExists() throws Exception {
- TestAccount user2 = accountCreator.user2();
-
- setAsDefaultCodeOwners(user);
-
- // Create a change as a user that is neither a code owner nor a project owner.
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange(user2, "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));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
-
- // Let the project owner approve the change.
- requestScopeOperations.setApiUser(admin.id());
- approve(changeId);
-
- // Verify that the file is not approved yet
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
-
- // Let the code owner approve the change.
- projectOperations
- .project(project)
- .forUpdate()
- .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
- .update();
- requestScopeOperations.setApiUser(user.id());
- approve(changeId);
-
- // Check that the file is approved now.
- requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1809,14 +1378,9 @@
// Verify that the file is not approved yet.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Let the code owner approve the change.
projectOperations
@@ -1830,13 +1394,8 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1864,14 +1423,13 @@
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -1889,14 +1447,9 @@
// Verify that the status of the file is INSUFFICIENT_REVIEWERS.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
// Add the default code owner as reviewer.
gApi.changes().id(changeId).addReviewer(user.email());
@@ -1904,13 +1457,8 @@
// Check that the status of the file is PENDING now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.PENDING));
// Let the default code owner approve the change.
projectOperations
@@ -1924,13 +1472,8 @@
// Check that the file is approved now.
requestScopeOperations.setApiUser(admin.id());
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1949,14 +1492,9 @@
// Check that the file is not approved.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeIdOfRevert));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -1976,14 +1514,8 @@
// Check that the file is approved since it's a pure revert.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeIdOfRevert));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.deletion(path, CodeOwnerStatus.APPROVED));
}
@Test
@@ -1998,11 +1530,16 @@
gApi.changes().id(changeId).current().submit();
// Revert the change
- String changeIdOfRevert = gApi.changes().id(changeId).revert().get().changeId;
+ ChangeInfo revertChange = gApi.changes().id(changeId).revert().get();
// Amend change to make it a non-pure revert change.
+ GitUtil.fetch(
+ testRepo,
+ RefNames.patchSetRef(PatchSet.id(Change.id(revertChange._number), 1)) + ":revert");
+ testRepo.reset("revert");
+
amendChange(
- changeIdOfRevert,
+ revertChange.changeId,
"refs/for/master",
admin,
testRepo,
@@ -2012,15 +1549,10 @@
// Check that the file is not approved.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeIdOfRevert));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(revertChange.changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.modification(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -2039,14 +1571,8 @@
// approvals.
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
// Amend the change by another user, so that the other non-exempted user becomes the last
// uploader.
@@ -2055,13 +1581,9 @@
// Check that the file is no longer approved since the uploader is not exempted from requiring
// code owner approvals.
fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- fileCodeOwnerStatusSubject = assertThatCollection(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
private ChangeNotes getChangeNotes(String changeId) throws Exception {
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
index 3878a27..d17d51b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
@@ -597,6 +597,51 @@
}
}
+ @Test
+ public void projectOwnersAreNotCodeOwnersIfDefaultCodeOwnerConfigExists() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ setAsDefaultCodeOwners(user);
+
+ // Create a change as a user that is neither a code owner nor a project owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user2, "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));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Let the project owner approve the change.
+ requestScopeOperations.setApiUser(admin.id());
+ approve(changeId);
+
+ // Verify that the file is not approved yet
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Let the code owner approve the change.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(user.id());
+ approve(changeId);
+
+ // Check that the file is approved now.
+ requestScopeOperations.setApiUser(admin.id());
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
+ }
+
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/CodeOwnerScoringsTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
index 1bb7090..daa2f41 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.IS_REVIEWER_SCORING_VALUE;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.NO_REVIEWER_SCORING_VALUE;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import java.util.ArrayList;
import org.junit.Test;
@@ -25,7 +26,7 @@
/** Tests for {@link CodeOwnerScorings}. */
public class CodeOwnerScoringsTest extends AbstractCodeOwnersTest {
@Test
- public void sortCodeOwnersByLowerValueIsBetterScore() throws Exception {
+ public void getScorings_lowerValueIsBetterScore() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
CodeOwner codeOwner3 = CodeOwner.create(accountCreator.user2().id());
@@ -43,12 +44,20 @@
.putValueForCodeOwner(codeOwner2, 100)
.putValueForCodeOwner(codeOwner3, 0)
.build();
- codeOwners.sort(CodeOwnerScorings.create(distanceScoring).comparingByScorings());
- assertThat(codeOwners).containsExactly(codeOwner3, codeOwner1, codeOwner2).inOrder();
+
+ CodeOwnerScorings codeOwnerScorings = CodeOwnerScorings.create(distanceScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2, codeOwner3)))
+ .containsExactly(
+ codeOwner1,
+ CodeOwnerScore.DISTANCE.weight() * 0.5,
+ codeOwner2,
+ 0.0,
+ codeOwner3,
+ CodeOwnerScore.DISTANCE.weight() * 1.0);
}
@Test
- public void sortCodeOwnersByGreaterValueIsBetterScore() throws Exception {
+ public void getScorings_greaterValueIsBetterScore() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
@@ -63,12 +72,18 @@
.putValueForCodeOwner(codeOwner1, 0)
.putValueForCodeOwner(codeOwner2, 1)
.build();
- codeOwners.sort(CodeOwnerScorings.create(isReviewerScoring).comparingByScorings());
- assertThat(codeOwners).containsExactly(codeOwner2, codeOwner1).inOrder();
+
+ CodeOwnerScorings codeOwnerScorings = CodeOwnerScorings.create(isReviewerScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2)))
+ .containsExactly(
+ codeOwner1,
+ CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.NO_REVIEWER_SCORING_VALUE,
+ codeOwner2,
+ CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.IS_REVIEWER_SCORING_VALUE);
}
@Test
- public void sortCodeOwnersByMultipleScore() throws Exception {
+ public void getScorings_multipleScore() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
CodeOwner codeOwner3 = CodeOwner.create(accountCreator.user2().id());
@@ -95,9 +110,6 @@
.putValueForCodeOwner(codeOwner3, NO_REVIEWER_SCORING_VALUE)
.build();
- codeOwners.sort(
- CodeOwnerScorings.create(distanceScoring, isReviewerScoring).comparingByScorings());
-
// Expected scorings:
// codeOwner1: DISTANCE(weight=1)=0.5, IS_REVIEWER(weight=2)=0.0
// -> total scoring: 0.5 * 1 + 0.0 * 2 = 0.5
@@ -105,12 +117,23 @@
// -> total scoring: 0.25 * 1 + 1.0 * 2 = 2.25
// codeOwner3: DISTANCE(weight=1)=1.0, IS_REVIEWER(weight=2)=0.0
// -> total scoring: 1.0 * 1 + 0.0 * 2 = 1.0
- // => expected order (highest total score first): codeOwner2, codeOwner3, codeOwner1
- assertThat(codeOwners).containsExactly(codeOwner2, codeOwner3, codeOwner1).inOrder();
+ CodeOwnerScorings codeOwnerScorings =
+ CodeOwnerScorings.create(distanceScoring, isReviewerScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2, codeOwner3)))
+ .containsExactly(
+ codeOwner1,
+ CodeOwnerScore.DISTANCE.weight() * 0.5
+ + CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.NO_REVIEWER_SCORING_VALUE,
+ codeOwner2,
+ CodeOwnerScore.DISTANCE.weight() * 0.25
+ + CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.IS_REVIEWER_SCORING_VALUE,
+ codeOwner3,
+ CodeOwnerScore.DISTANCE.weight() * 1.0
+ + CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.NO_REVIEWER_SCORING_VALUE);
}
@Test
- public void sortCodeOwnersByScoringsIfAnyCodeOwnerHasNoScoring() throws Exception {
+ public void getScoringsIfAnyCodeOwnerHasNoScoring() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
@@ -122,7 +145,9 @@
CodeOwnerScoring.builder(CodeOwnerScore.DISTANCE, 100)
.putValueForCodeOwner(codeOwner2, 50)
.build();
- codeOwners.sort(CodeOwnerScorings.create(distanceScoring).comparingByScorings());
- assertThat(codeOwners).containsExactly(codeOwner2, codeOwner1).inOrder();
+
+ CodeOwnerScorings codeOwnerScorings = CodeOwnerScorings.create(distanceScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2)))
+ .containsExactly(codeOwner1, 0.0, codeOwner2, 0.5);
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
index dc709aa..e493e40 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
@@ -14,8 +14,10 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.SubmitRecordSubject.assertThatOptional;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -120,7 +122,7 @@
}
@Test
- public void ruleError() throws Exception {
+ public void internalServerError() throws Exception {
ChangeData changeData = createChange().getChange();
// Create a ChangeData without change notes to trigger an error.
@@ -129,11 +131,12 @@
when(changeDataWithoutChangeNotes.change()).thenReturn(changeData.change());
when(changeDataWithoutChangeNotes.currentPatchSet()).thenReturn(changeData.currentPatchSet());
- SubmitRecordSubject submitRecordSubject =
- assertThatOptional(codeOwnerSubmitRule.evaluate(changeDataWithoutChangeNotes)).value();
- submitRecordSubject.hasStatusThat().isRuleError();
- submitRecordSubject
- .hasErrorMessageThat()
+ CodeOwnersInternalServerErrorException exception =
+ assertThrows(
+ CodeOwnersInternalServerErrorException.class,
+ () -> codeOwnerSubmitRule.evaluate(changeDataWithoutChangeNotes));
+ assertThat(exception)
+ .hasMessageThat()
.isEqualTo(
String.format(
"Failed to evaluate code owner statuses for patch set %d of change %d.",
@@ -141,11 +144,12 @@
}
@Test
- public void ruleError_changeDataIsNull() throws Exception {
- SubmitRecordSubject submitRecordSubject =
- assertThatOptional(codeOwnerSubmitRule.evaluate(/* changeData= */ null)).value();
- submitRecordSubject.hasStatusThat().isRuleError();
- submitRecordSubject.hasErrorMessageThat().isEqualTo("Failed to evaluate code owner statuses.");
+ public void internalServerError_changeDataIsNull() throws Exception {
+ CodeOwnersInternalServerErrorException exception =
+ assertThrows(
+ CodeOwnersInternalServerErrorException.class,
+ () -> codeOwnerSubmitRule.evaluate(/* changeData= */ null));
+ assertThat(exception).hasMessageThat().isEqualTo("Failed to evaluate code owner statuses.");
}
@Test
diff --git a/resources/Documentation/config-guide.md b/resources/Documentation/config-guide.md
index f96c2bc..ab8b46a 100644
--- a/resources/Documentation/config-guide.md
+++ b/resources/Documentation/config-guide.md
@@ -179,10 +179,18 @@
If [implicit approvals](#implicitApprovals) are enabled, it is important that
code owners are aware of their implicit approval when they upload new patch sets
-for other users. E.g. if a contributor pushes a change to a wrong branch and a
-code owner helps them to get it rebased onto the correct branch, the rebased
-change has implicit approvals from the code owner, since the code owner is the
-uploader. To avoid situations like this it is recommended to not enable implicit
+for other users.
+
+Examples:
+
+* If a contributor pushes a change to a wrong branch and a code owner helps them
+ to get it rebased onto the correct branch, the rebased change has implicit
+ approvals from the code owner, since the code owner is the uploader.
+* If a code owner edits the commit message of change owned by a non-code-owner,
+ the change gets implicitly approved since editing the commit message creates a
+ new patch set and the code owner is the uploader that patch set.
+
+To avoid situations like this it is recommended to not enable implicit
approvals.
### <a id="securityMergeCommits">Required code owner approvals on merge commits
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 5cff902..0fc2673 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -3,6 +3,8 @@
The @PLUGIN@ plugin exports several metrics which can give insights into the
usage and performance of the code owners functionality.
+All metrics have the following prefix: `plugins/@PLUGIN@/`
+
## <a id="latencyMetrics"> Latency Metrics
* `add_change_message_on_add_reviewer`:
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 40fb545..c937cb2 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -459,6 +459,7 @@
| `limit`\|`n` | optional | Limit defining how many code owners should be returned at most. By default 10.
| `seed` | optional | Seed, as a long value, that should be used to shuffle code owners that have the same score. Can be used to make the sort order stable across several requests, e.g. to get the same set of random code owners for different file paths that have the same code owners. Important: the sort order is only stable if the requests use the same seed **and** the same limit. In addition, the sort order is not guaranteed to be stable if new accounts are created in between the requests, or if the account visibility is changed.
| `resolve-all-users` | optional | Whether code ownerships that are assigned to all users should be resolved to random users. If not set, `false` by default. Also see the [sorting example](#sortingExample) below to see how this parameter affects the returned code owners.
+| `highest-score-only` | optional | Whether only code owners with the highest score should be returned. If not set, `false` by default.
| `revision` | optional | Revision from which the code owner configs should be read as commit SHA1. Can be used to read historic code owners from this branch, but imports from other branches or repositories as well as default and global code owners from `refs/meta/config` are still read from the current revisions. If not specified the code owner configs are read from the HEAD revision of the branch. Not supported for getting code owners for a path in a change.
As a response a [CodeOwnersInfo](#code-owners-info) entity is returned that
@@ -657,9 +658,11 @@
The following request parameters can be specified:
-| Field Name | | Description |
-| ----------- | --------- | ----------- |
-| `user` | mandatory | user for which the owned paths should be returned
+| Field Name | | Description |
+| ------------ | --------- | ----------- |
+| `start`\|`S` | optional | Number of owned paths to skip. Allows to page over the owned files. By default 0.
+| `limit`\|`n` | optional | Limit defining how many owned files should be returned at most. By default 50.
+| `user` | mandatory | user for which the owned paths should be returned
#### Request
@@ -960,9 +963,10 @@
The `OwnedPathsInfo` entity contains paths that are owned by a user.
-| Field Name | Description |
-| ------------- | ----------- |
-| `owned_paths` | The owned paths as absolute paths, sorted alphabetically.
+| Field Name | | Description |
+| ------------- | -------- | ----------- |
+| `owned_paths` | |The owned paths as absolute paths, sorted alphabetically.
+| `more` | optional | Whether the request would deliver more results if not limited. Not set if `false`.
### <a id="path-code-owner-status-info"> PathCodeOwnerStatusInfo
The `PathCodeOwnerStatusInfo` entity describes the code owner status for a path
