From af6146c03b7df646a0ae38a6484821a7e6a34f40 Mon Sep 17 00:00:00 2001 From: Logan Hanks Date: Sun, 28 May 2017 12:45:44 -0700 Subject: [PATCH] Improve performance of notification sender tests Creating all the StagedUsers accounts is expensive. This change makes it possible to reuse these test accounts across test methods within the same class, saving about 10% of test execution time. This change also saves a little time by adding reviewers to staged changes in a single batch operation, rather than one API call at a time. Other slight gains were achieved by having watchers watch All-Projects, instead of watching a new project for every test method. Change-Id: I413c8f98338be30dcf926a724333feb401c9aa52 --- .../gerrit/acceptance/AbstractDaemonTest.java | 2 + .../acceptance/AbstractNotificationTest.java | 139 ++++++++++++------ .../server/mail/CommentSenderIT.java | 49 +++--- .../server/mail/SetAssigneeSenderIT.java | 36 ++--- .../gerrit/server/git/ReceiveCommits.java | 7 +- 5 files changed, 143 insertions(+), 90 deletions(-) diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 07d39be555..bf40dda3c4 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -236,6 +236,7 @@ public abstract class AbstractDaemonTest { protected TestAccount user; protected TestRepository testRepo; protected String resourcePrefix; + protected Description description; @Inject private ChangeIndexCollection changeIndexes; @Inject private EventRecorder.Factory eventRecorderFactory; @@ -308,6 +309,7 @@ public abstract class AbstractDaemonTest { } protected void beforeTest(Description description) throws Exception { + this.description = description; GerritServer.Description classDesc = GerritServer.Description.forTestClass(description, configName); GerritServer.Description methodDesc = diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractNotificationTest.java index 416d859fa5..a962c70c85 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractNotificationTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractNotificationTest.java @@ -24,9 +24,9 @@ import com.google.common.truth.Subject; import com.google.common.truth.SubjectFactory; import com.google.common.truth.Truth; import com.google.gerrit.common.Nullable; -import com.google.gerrit.extensions.api.changes.AddReviewerInput; -import com.google.gerrit.extensions.api.changes.AddReviewerResult; import com.google.gerrit.extensions.api.changes.RecipientType; +import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.api.changes.ReviewResult; import com.google.gerrit.extensions.api.projects.ConfigInput; import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy; @@ -38,11 +38,15 @@ import com.google.gerrit.server.mail.send.EmailHeader; import com.google.gerrit.server.mail.send.EmailHeader.AddressList; import com.google.gerrit.testutil.FakeEmailSender; import com.google.gerrit.testutil.FakeEmailSender.Message; +import java.io.IOException; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; import org.eclipse.jgit.junit.TestRepository; +import org.junit.After; import org.junit.Before; public abstract class AbstractNotificationTest extends AbstractDaemonTest { @@ -69,6 +73,14 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { } protected void setEmailStrategy(TestAccount account, EmailStrategy strategy) throws Exception { + setEmailStrategy(account, strategy, true); + } + + protected void setEmailStrategy(TestAccount account, EmailStrategy strategy, boolean record) + throws Exception { + if (record) { + accountsModifyingEmailStrategy.add(account); + } setApiUser(account); GeneralPreferencesInfo prefs = gApi.accounts().self().getPreferences(); prefs.emailStrategy = strategy; @@ -241,6 +253,20 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { } } + private static final Map stagedUsers = new HashMap<>(); + + // TestAccount doesn't implement hashCode/equals, so this set is according + // to object identity. That's fine for our purposes. + private Set accountsModifyingEmailStrategy = new HashSet<>(); + + @After + public void resetEmailStrategies() throws Exception { + for (TestAccount account : accountsModifyingEmailStrategy) { + setEmailStrategy(account, EmailStrategy.ENABLED, false); + } + accountsModifyingEmailStrategy.clear(); + } + protected class StagedUsers { public final TestAccount owner; public final TestAccount author; @@ -248,6 +274,7 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { public final TestAccount reviewer; public final TestAccount ccer; public final TestAccount starrer; + public final TestAccount assignee; public final TestAccount watchingProjectOwner; public final String reviewerByEmail = "reviewerByEmail@example.com"; public final String ccerByEmail = "ccByEmail@example.com"; @@ -255,31 +282,59 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { private final Map accountsByEmail = new HashMap<>(); boolean supportReviewersByEmail; - StagedUsers(List watches) throws Exception { - owner = testAccount("owner"); - reviewer = testAccount("reviewer"); - author = testAccount("author"); - uploader = testAccount("uploader"); - ccer = testAccount("ccer"); - starrer = testAccount("starrer"); + private String usersCacheKey() { + return description.getClassName(); + } - watchingProjectOwner = testAccount("watchingProjectOwner", "Administrators"); - setApiUser(watchingProjectOwner); - watch(project.get(), pwi -> pwi.notifyNewChanges = true); + private TestAccount evictAndCopy(TestAccount account) throws IOException { + accountCache.evict(account.id); + return account; + } - for (NotifyType watch : watches) { - TestAccount watcher = testAccount(watch.toString()); - setApiUser(watcher); - watch( - project.get(), - pwi -> { - pwi.notifyAllComments = watch.equals(NotifyType.ALL_COMMENTS); - pwi.notifyAbandonedChanges = watch.equals(NotifyType.ABANDONED_CHANGES); - pwi.notifyNewChanges = watch.equals(NotifyType.NEW_CHANGES); - pwi.notifyNewPatchSets = watch.equals(NotifyType.NEW_PATCHSETS); - pwi.notifySubmittedChanges = watch.equals(NotifyType.SUBMITTED_CHANGES); - }); - watchers.put(watch, watcher); + public StagedUsers(List watches) throws Exception { + synchronized (stagedUsers) { + if (stagedUsers.containsKey(usersCacheKey())) { + StagedUsers existing = stagedUsers.get(usersCacheKey()); + owner = evictAndCopy(existing.owner); + author = evictAndCopy(existing.author); + uploader = evictAndCopy(existing.uploader); + reviewer = evictAndCopy(existing.reviewer); + ccer = evictAndCopy(existing.ccer); + starrer = evictAndCopy(existing.starrer); + assignee = evictAndCopy(existing.assignee); + watchingProjectOwner = evictAndCopy(existing.watchingProjectOwner); + watchers.putAll(existing.watchers); + return; + } + + owner = testAccount("owner"); + reviewer = testAccount("reviewer"); + author = testAccount("author"); + uploader = testAccount("uploader"); + ccer = testAccount("ccer"); + starrer = testAccount("starrer"); + assignee = testAccount("assignee"); + + watchingProjectOwner = testAccount("watchingProjectOwner", "Administrators"); + setApiUser(watchingProjectOwner); + watch(allProjects.get(), pwi -> pwi.notifyNewChanges = true); + + for (NotifyType watch : watches) { + TestAccount watcher = testAccount(watch.toString()); + setApiUser(watcher); + watch( + allProjects.get(), + pwi -> { + pwi.notifyAllComments = watch.equals(NotifyType.ALL_COMMENTS); + pwi.notifyAbandonedChanges = watch.equals(NotifyType.ABANDONED_CHANGES); + pwi.notifyNewChanges = watch.equals(NotifyType.NEW_CHANGES); + pwi.notifyNewPatchSets = watch.equals(NotifyType.NEW_PATCHSETS); + pwi.notifySubmittedChanges = watch.equals(NotifyType.SUBMITTED_CHANGES); + }); + watchers.put(watch, watcher); + } + + stagedUsers.put(usersCacheKey(), this); } } @@ -316,25 +371,23 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { } protected void addReviewers(PushOneCommit.Result r) throws Exception { - AddReviewerInput in = new AddReviewerInput(); - in.reviewer = reviewer.email; - gApi.changes().id(r.getChangeId()).addReviewer(in); - - in.reviewer = ccer.email; - in.state = ReviewerState.CC; - gApi.changes().id(r.getChangeId()).addReviewer(in); - - in.reviewer = reviewerByEmail; - in.state = ReviewerState.REVIEWER; - AddReviewerResult result = gApi.changes().id(r.getChangeId()).addReviewer(in); - if (result.reviewers == null || result.reviewers.isEmpty()) { + ReviewInput in = + ReviewInput.noScore() + .reviewer(reviewer.email) + .reviewer(reviewerByEmail) + .reviewer(ccer.email, ReviewerState.CC, false) + .reviewer(ccerByEmail, ReviewerState.CC, false); + ReviewResult result = gApi.changes().id(r.getChangeId()).revision("current").review(in); + supportReviewersByEmail = true; + if (result.reviewers.values().stream().anyMatch(v -> v.error != null)) { supportReviewersByEmail = false; - } else { - supportReviewersByEmail = true; - in.reviewer = ccerByEmail; - in.state = ReviewerState.CC; - gApi.changes().id(r.getChangeId()).addReviewer(in); + in = + ReviewInput.noScore() + .reviewer(reviewer.email) + .reviewer(ccer.email, ReviewerState.CC, false); + result = gApi.changes().id(r.getChangeId()).revision("current").review(in); } + Truth.assertThat(result.reviewers.values().stream().allMatch(v -> v.error == null)).isTrue(); } } @@ -362,6 +415,7 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { if (pushOptions != null) { ref = ref + '%' + Joiner.on(',').join(pushOptions); } + setApiUser(owner); repo = cloneProject(project, owner); PushOneCommit push = pushFactory.create(db, owner.getIdent(), repo); result = push.to(ref); @@ -381,7 +435,6 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { } protected class StagedChange extends StagedPreChange { - StagedChange(String ref, List watches) throws Exception { super(ref, watches); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/CommentSenderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/CommentSenderIT.java index e78798f233..3123440d73 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/CommentSenderIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/CommentSenderIT.java @@ -27,7 +27,6 @@ import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.ReviewInput; -import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy; import org.junit.Test; @@ -35,8 +34,7 @@ public class CommentSenderIT extends AbstractNotificationTest { @Test public void commentOnReviewableChangeByOwner() throws Exception { StagedChange sc = stageReviewableChange(ALL_COMMENTS); - setApiUser(sc.owner); - review(sc.changeId, ENABLED); + review(sc.owner, sc.changeId, ENABLED); assertThat(sender) .sent("comment", sc) .notTo(sc.owner) @@ -50,8 +48,7 @@ public class CommentSenderIT extends AbstractNotificationTest { @Test public void commentOnReviewableChangeByReviewer() throws Exception { StagedChange sc = stageReviewableChange(ALL_COMMENTS); - setApiUser(sc.reviewer); - review(sc.changeId, ENABLED); + review(sc.reviewer, sc.changeId, ENABLED); assertThat(sender) .sent("comment", sc) .notTo(sc.reviewer) @@ -66,8 +63,7 @@ public class CommentSenderIT extends AbstractNotificationTest { @Test public void commentOnReviewableChangeByOwnerCcingSelf() throws Exception { StagedChange sc = stageReviewableChange(ALL_COMMENTS); - setApiUser(sc.owner); - review(sc.changeId, CC_ON_OWN_COMMENTS); + review(sc.owner, sc.changeId, CC_ON_OWN_COMMENTS); assertThat(sender) .sent("comment", sc) .to(sc.owner) @@ -81,8 +77,7 @@ public class CommentSenderIT extends AbstractNotificationTest { @Test public void commentOnReviewableChangeByReviewerCcingSelf() throws Exception { StagedChange sc = stageReviewableChange(ALL_COMMENTS); - setApiUser(sc.reviewer); - review(sc.changeId, CC_ON_OWN_COMMENTS); + review(sc.reviewer, sc.changeId, CC_ON_OWN_COMMENTS); assertThat(sender) .sent("comment", sc) .to(sc.owner) @@ -97,8 +92,7 @@ public class CommentSenderIT extends AbstractNotificationTest { public void commentOnReviewableChangeByOther() throws Exception { TestAccount other = accountCreator.create("other", "other@example.com", "other"); StagedChange sc = stageReviewableChange(ALL_COMMENTS); - setApiUser(other); - review(sc.changeId, ENABLED); + review(other, sc.changeId, ENABLED); assertThat(sender) .sent("comment", sc) .notTo(other) @@ -114,8 +108,7 @@ public class CommentSenderIT extends AbstractNotificationTest { public void commentOnReviewableChangeByOtherCcingSelf() throws Exception { TestAccount other = accountCreator.create("other", "other@example.com", "other"); StagedChange sc = stageReviewableChange(ALL_COMMENTS); - setApiUser(other); - review(sc.changeId, CC_ON_OWN_COMMENTS); + review(other, sc.changeId, CC_ON_OWN_COMMENTS); assertThat(sender) .sent("comment", sc) .to(sc.owner) @@ -129,7 +122,7 @@ public class CommentSenderIT extends AbstractNotificationTest { @Test public void commentOnReviewableChangeByOwnerNotifyOwnerReviewers() throws Exception { StagedChange sc = stageReviewableChange(ALL_COMMENTS); - review(sc.changeId, ENABLED, OWNER_REVIEWERS); + review(sc.owner, sc.changeId, ENABLED, OWNER_REVIEWERS); assertThat(sender) .sent("comment", sc) .to(sc.reviewerByEmail) // TODO(logan): This is unintentionally TO, should be CC. @@ -142,7 +135,7 @@ public class CommentSenderIT extends AbstractNotificationTest { @Test public void commentOnReviewableChangeByOwnerNotifyOwner() throws Exception { StagedChange sc = stageReviewableChange(ALL_COMMENTS); - review(sc.changeId, ENABLED, OWNER); + review(sc.owner, sc.changeId, ENABLED, OWNER); assertThat(sender).notSent(); } @@ -150,14 +143,14 @@ public class CommentSenderIT extends AbstractNotificationTest { public void commentOnReviewableChangeByOwnerCcingSelfNotifyOwner() throws Exception { StagedChange sc = stageReviewableChange(ALL_COMMENTS); setEmailStrategy(sc.owner, CC_ON_OWN_COMMENTS); - review(sc.changeId, ENABLED, OWNER); + review(sc.owner, sc.changeId, ENABLED, OWNER); assertThat(sender).notSent(); // TODO(logan): Why not send to owner? } @Test public void commentOnReviewableChangeByOwnerNotifyNone() throws Exception { StagedChange sc = stageReviewableChange(ALL_COMMENTS); - review(sc.changeId, ENABLED, NONE); + review(sc.owner, sc.changeId, ENABLED, NONE); assertThat(sender).notSent(); } @@ -165,28 +158,28 @@ public class CommentSenderIT extends AbstractNotificationTest { public void commentOnReviewableChangeByOwnerCcingSelfNotifyNone() throws Exception { StagedChange sc = stageReviewableChange(ALL_COMMENTS); setEmailStrategy(sc.owner, CC_ON_OWN_COMMENTS); - review(sc.changeId, ENABLED, NONE); + review(sc.owner, sc.changeId, ENABLED, NONE); assertThat(sender).notSent(); // TODO(logan): Why not send to owner? } @Test public void commentOnWipChangeByOwner() throws Exception { StagedChange sc = stageWipChange(ALL_COMMENTS); - review(sc.changeId, ENABLED); + review(sc.owner, sc.changeId, ENABLED); assertThat(sender).notSent(); } @Test public void commentOnWipChangeByOwnerCcingSelf() throws Exception { StagedChange sc = stageWipChange(ALL_COMMENTS); - review(sc.changeId, CC_ON_OWN_COMMENTS); + review(sc.owner, sc.changeId, CC_ON_OWN_COMMENTS); assertThat(sender).notSent(); } @Test public void commentOnWipChangeByOwnerNotifyAll() throws Exception { StagedChange sc = stageWipChange(ALL_COMMENTS); - review(sc.changeId, ENABLED, ALL); + review(sc.owner, sc.changeId, ENABLED, ALL); assertThat(sender) .sent("comment", sc) .notTo(sc.owner) @@ -200,19 +193,19 @@ public class CommentSenderIT extends AbstractNotificationTest { @Test public void commentOnReviewableWipChangeByOwner() throws Exception { StagedChange sc = stageReviewableWipChange(ALL_COMMENTS); - review(sc.changeId, ENABLED); + review(sc.owner, sc.changeId, ENABLED); assertThat(sender).notSent(); } - private void review(String changeId, EmailStrategy strategy) throws Exception { - review(changeId, strategy, null); + private void review(TestAccount account, String changeId, EmailStrategy strategy) + throws Exception { + review(account, changeId, strategy, null); } - private void review(String changeId, EmailStrategy strategy, @Nullable NotifyHandling notify) + private void review( + TestAccount account, String changeId, EmailStrategy strategy, @Nullable NotifyHandling notify) throws Exception { - GeneralPreferencesInfo prefs = gApi.accounts().self().getPreferences(); - prefs.emailStrategy = strategy; - gApi.accounts().self().setPreferences(prefs); + setEmailStrategy(account, strategy); ReviewInput in = ReviewInput.recommend(); in.notify = notify; gApi.changes().id(changeId).revision("current").review(in); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/SetAssigneeSenderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/SetAssigneeSenderIT.java index 2d3d272059..37c9d8b1f7 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/SetAssigneeSenderIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/SetAssigneeSenderIT.java @@ -35,51 +35,51 @@ public class SetAssigneeSenderIT extends AbstractNotificationTest { @Test public void setAssigneeOnReviewableChange() throws Exception { StagedChange sc = stageReviewableChange(); - assign(sc, sc.owner, assignee); + assign(sc, sc.owner, sc.assignee); assertThat(sender) .sent("setassignee", sc) .notTo(sc.owner, sc.reviewer, sc.ccer, sc.starrer) .to(sc.reviewerByEmail) // TODO(logan): This is probably not intended! .cc(sc.ccerByEmail) // TODO(logan): This is probably not intended! - .to(assignee); + .to(sc.assignee); } @Test public void setAssigneeOnReviewableChangeByOwnerCcingSelf() throws Exception { StagedChange sc = stageReviewableChange(); - assign(sc, sc.owner, assignee, CC_ON_OWN_COMMENTS); + assign(sc, sc.owner, sc.assignee, CC_ON_OWN_COMMENTS); assertThat(sender) .sent("setassignee", sc) .notTo(sc.reviewer, sc.ccer, sc.starrer) .cc(sc.owner) .to(sc.reviewerByEmail) // TODO(logan): This is probably not intended! .cc(sc.ccerByEmail) // TODO(logan): This is probably not intended! - .to(assignee); + .to(sc.assignee); } @Test public void setAssigneeOnReviewableChangeByAdmin() throws Exception { StagedChange sc = stageReviewableChange(); - assign(sc, admin, assignee); + assign(sc, admin, sc.assignee); assertThat(sender) .sent("setassignee", sc) .notTo(sc.owner, sc.reviewer, sc.ccer, sc.starrer, admin) .to(sc.reviewerByEmail) // TODO(logan): This is probably not intended! .cc(sc.ccerByEmail) // TODO(logan): This is probably not intended! - .to(assignee); + .to(sc.assignee); } @Test public void setAssigneeOnReviewableChangeByAdminCcingSelf() throws Exception { StagedChange sc = stageReviewableChange(); - assign(sc, admin, assignee, CC_ON_OWN_COMMENTS); + assign(sc, admin, sc.assignee, CC_ON_OWN_COMMENTS); assertThat(sender) .sent("setassignee", sc) .notTo(sc.owner, sc.reviewer, sc.ccer, sc.starrer) .cc(admin) .to(sc.reviewerByEmail) // TODO(logan): This is probably not intended! .cc(sc.ccerByEmail) // TODO(logan): This is probably not intended! - .to(assignee); + .to(sc.assignee); } @Test @@ -89,7 +89,7 @@ public class SetAssigneeSenderIT extends AbstractNotificationTest { assign(sc, sc.owner, sc.owner); assertThat(sender) .sent("setassignee", sc) - .notTo(sc.owner, sc.reviewer, sc.ccer, sc.starrer, assignee) + .notTo(sc.owner, sc.reviewer, sc.ccer, sc.starrer, sc.assignee) .to(sc.reviewerByEmail) // TODO(logan): This is probably not intended! .cc(sc.ccerByEmail); // TODO(logan): This is probably not intended! } @@ -108,25 +108,25 @@ public class SetAssigneeSenderIT extends AbstractNotificationTest { TestAccount other = accountCreator.create("other", "other@example.com", "other"); assign(sc, sc.owner, other); sender.clear(); - assign(sc, sc.owner, assignee); + assign(sc, sc.owner, sc.assignee); assertThat(sender) .sent("setassignee", sc) .notTo(sc.owner, sc.reviewer, sc.ccer, sc.starrer, other) .to(sc.reviewerByEmail) // TODO(logan): This is probably not intended! .cc(sc.ccerByEmail) // TODO(logan): This is probably not intended! - .to(assignee); + .to(sc.assignee); } @Test public void changeAssigneeToSelfOnReviewableChangeInNoteDb() throws Exception { assume().that(notesMigration.readChanges()).isTrue(); StagedChange sc = stageReviewableChange(); - assign(sc, sc.owner, assignee); + assign(sc, sc.owner, sc.assignee); sender.clear(); assign(sc, sc.owner, sc.owner); assertThat(sender) .sent("setassignee", sc) - .notTo(sc.owner, sc.reviewer, sc.ccer, sc.starrer, assignee) + .notTo(sc.owner, sc.reviewer, sc.ccer, sc.starrer, sc.assignee) .to(sc.reviewerByEmail) // TODO(logan): This is probably not intended! .cc(sc.ccerByEmail); // TODO(logan): This is probably not intended! } @@ -135,7 +135,7 @@ public class SetAssigneeSenderIT extends AbstractNotificationTest { public void changeAssigneeToSelfOnReviewableChangeInReviewDb() throws Exception { assume().that(notesMigration.readChanges()).isFalse(); StagedChange sc = stageReviewableChange(); - assign(sc, sc.owner, assignee); + assign(sc, sc.owner, sc.assignee); sender.clear(); assign(sc, sc.owner, sc.owner); assertThat(sender).notSent(); @@ -144,25 +144,25 @@ public class SetAssigneeSenderIT extends AbstractNotificationTest { @Test public void setAssigneeOnReviewableWipChange() throws Exception { StagedChange sc = stageReviewableWipChange(); - assign(sc, sc.owner, assignee); + assign(sc, sc.owner, sc.assignee); assertThat(sender) .sent("setassignee", sc) .notTo(sc.owner, sc.reviewer, sc.ccer, sc.starrer) .to(sc.reviewerByEmail) // TODO(logan): This is probably not intended! .cc(sc.ccerByEmail) // TODO(logan): This is probably not intended! - .to(assignee); + .to(sc.assignee); } @Test public void setAssigneeOnWipChange() throws Exception { StagedChange sc = stageWipChange(); - assign(sc, sc.owner, assignee); + assign(sc, sc.owner, sc.assignee); assertThat(sender) .sent("setassignee", sc) .notTo(sc.owner, sc.reviewer, sc.ccer, sc.starrer) .to(sc.reviewerByEmail) // TODO(logan): This is probably not intended! .cc(sc.ccerByEmail) // TODO(logan): This is probably not intended! - .to(assignee); + .to(sc.assignee); } private void assign(StagedChange sc, TestAccount by, TestAccount to) throws Exception { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 11330cb294..585c8e2775 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -60,6 +60,7 @@ import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.api.projects.ProjectConfigEntryType; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.registration.DynamicMap.Entry; import com.google.gerrit.extensions.registration.DynamicSet; @@ -1358,8 +1359,12 @@ public class ReceiveCommits { this.draft = cmd.getRefName().startsWith(MagicBranch.NEW_DRAFT_CHANGE); this.labelTypes = labelTypes; this.notesMigration = notesMigration; + GeneralPreferencesInfo prefs = user.getAccount().getGeneralPreferencesInfo(); this.defaultPublishComments = - firstNonNull(user.getAccount().getGeneralPreferencesInfo().publishCommentsOnPush, false); + prefs != null + ? firstNonNull( + user.getAccount().getGeneralPreferencesInfo().publishCommentsOnPush, false) + : false; } MailRecipients getMailRecipients() {