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-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -236,6 +236,7 @@ public abstract class AbstractDaemonTest {
   protected TestAccount user;
   protected TestRepository<InMemoryRepository> 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 =

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<String, StagedUsers> 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<TestAccount> 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<String, TestAccount> accountsByEmail = new HashMap<>();
     boolean supportReviewersByEmail;
 
-    StagedUsers(List<NotifyType> watches) throws Exception {
+    private String usersCacheKey() {
-      owner = testAccount("owner");
+      return description.getClassName();
-      reviewer = testAccount("reviewer");
+    }
-      author = testAccount("author");
-      uploader = testAccount("uploader");
-      ccer = testAccount("ccer");
-      starrer = testAccount("starrer");
 
-      watchingProjectOwner = testAccount("watchingProjectOwner", "Administrators");
+    private TestAccount evictAndCopy(TestAccount account) throws IOException {
-      setApiUser(watchingProjectOwner);
+      accountCache.evict(account.id);
-      watch(project.get(), pwi -> pwi.notifyNewChanges = true);
+      return account;
+    }
 
-      for (NotifyType watch : watches) {
+    public StagedUsers(List<NotifyType> watches) throws Exception {
-        TestAccount watcher = testAccount(watch.toString());
+      synchronized (stagedUsers) {
-        setApiUser(watcher);
+        if (stagedUsers.containsKey(usersCacheKey())) {
-        watch(
+          StagedUsers existing = stagedUsers.get(usersCacheKey());
-            project.get(),
+          owner = evictAndCopy(existing.owner);
-            pwi -> {
+          author = evictAndCopy(existing.author);
-              pwi.notifyAllComments = watch.equals(NotifyType.ALL_COMMENTS);
+          uploader = evictAndCopy(existing.uploader);
-              pwi.notifyAbandonedChanges = watch.equals(NotifyType.ABANDONED_CHANGES);
+          reviewer = evictAndCopy(existing.reviewer);
-              pwi.notifyNewChanges = watch.equals(NotifyType.NEW_CHANGES);
+          ccer = evictAndCopy(existing.ccer);
-              pwi.notifyNewPatchSets = watch.equals(NotifyType.NEW_PATCHSETS);
+          starrer = evictAndCopy(existing.starrer);
-              pwi.notifySubmittedChanges = watch.equals(NotifyType.SUBMITTED_CHANGES);
+          assignee = evictAndCopy(existing.assignee);
-            });
+          watchingProjectOwner = evictAndCopy(existing.watchingProjectOwner);
-        watchers.put(watch, watcher);
+          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();
+      ReviewInput in =
-      in.reviewer = reviewer.email;
+          ReviewInput.noScore()
-      gApi.changes().id(r.getChangeId()).addReviewer(in);
+              .reviewer(reviewer.email)
-
+              .reviewer(reviewerByEmail)
-      in.reviewer = ccer.email;
+              .reviewer(ccer.email, ReviewerState.CC, false)
-      in.state = ReviewerState.CC;
+              .reviewer(ccerByEmail, ReviewerState.CC, false);
-      gApi.changes().id(r.getChangeId()).addReviewer(in);
+      ReviewResult result = gApi.changes().id(r.getChangeId()).revision("current").review(in);
-
+      supportReviewersByEmail = true;
-      in.reviewer = reviewerByEmail;
+      if (result.reviewers.values().stream().anyMatch(v -> v.error != null)) {
-      in.state = ReviewerState.REVIEWER;
-      AddReviewerResult result = gApi.changes().id(r.getChangeId()).addReviewer(in);
-      if (result.reviewers == null || result.reviewers.isEmpty()) {
         supportReviewersByEmail = false;
-      } else {
+        in =
-        supportReviewersByEmail = true;
+            ReviewInput.noScore()
-        in.reviewer = ccerByEmail;
+                .reviewer(reviewer.email)
-        in.state = ReviewerState.CC;
+                .reviewer(ccer.email, ReviewerState.CC, false);
-        gApi.changes().id(r.getChangeId()).addReviewer(in);
+        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<NotifyType> watches) throws Exception {
       super(ref, watches);
 

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.owner, sc.changeId, ENABLED);
-    review(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.reviewer, sc.changeId, ENABLED);
-    review(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.owner, sc.changeId, CC_ON_OWN_COMMENTS);
-    review(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.reviewer, sc.changeId, CC_ON_OWN_COMMENTS);
-    review(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(other, sc.changeId, ENABLED);
-    review(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(other, sc.changeId, CC_ON_OWN_COMMENTS);
-    review(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 {
+  private void review(TestAccount account, String changeId, EmailStrategy strategy)
-    review(changeId, strategy, null);
+      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();
+    setEmailStrategy(account, strategy);
-    prefs.emailStrategy = strategy;
-    gApi.accounts().self().setPreferences(prefs);
     ReviewInput in = ReviewInput.recommend();
     in.notify = notify;
     gApi.changes().id(changeId).revision("current").review(in);

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 {

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() {