Fix NoteDb small tests to not depend on PrimaryStorage
The shouldExist argument to ChangeNotes allows loading a ChangeNotes for
a change that is yet to be created. It was incorrect for the updates
created by TestChanges#newUpdate to always set shouldExist=true; the
reality is that some tests require shouldExist=false, when creating the
first update for a new change. It was an accident that this worked at
all, since it was depending on the codepath where
PrimaryStorage=REVIEW_DB, which was for writing incomplete change
metadata to NoteDb during a migration. This migration code needs to go
away, so we can no longer rely on this accident.
Tweak the factory methods in AbstractChangeNotesTest to explicitly
specify that some updates are for new changes.
Change-Id: Ib6d30014814990ff5481a6bdf355c4ad63f66b85
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 37de143..acb6572 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -51,7 +51,6 @@
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
import com.google.gerrit.server.git.RefCache;
-import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
@@ -113,7 +112,7 @@
throws OrmException {
// Prepopulate the change exists with proper noteDbState field.
Change change = newChange(project, changeId);
- return new ChangeNotes(args, change).load();
+ return new ChangeNotes(args, change, true, null).load();
}
public ChangeNotes createChecked(Change.Id changeId) throws OrmException {
@@ -139,7 +138,7 @@
public ChangeNotes create(Project.NameKey project, Change.Id changeId) throws OrmException {
checkArgument(project != null, "project is required");
- return new ChangeNotes(args, newChange(project, changeId)).load();
+ return new ChangeNotes(args, newChange(project, changeId), true, null).load();
}
/**
@@ -150,7 +149,7 @@
* @return change notes
*/
public ChangeNotes createFromIndexedChange(Change change) {
- return new ChangeNotes(args, change);
+ return new ChangeNotes(args, change, true, null);
}
public ChangeNotes createForBatchUpdate(Change change, boolean shouldExist)
@@ -232,7 +231,7 @@
@Nullable
private ChangeNotesResult toResult(Change rawChangeFromNoteDb) {
- ChangeNotes n = new ChangeNotes(args, rawChangeFromNoteDb);
+ ChangeNotes n = new ChangeNotes(args, rawChangeFromNoteDb, true, null);
try {
n.load();
} catch (OrmException e) {
@@ -319,11 +318,7 @@
private ImmutableSet<Comment.Key> commentKeys;
@VisibleForTesting
- public ChangeNotes(Args args, Change change) {
- this(args, change, true, null);
- }
-
- private ChangeNotes(Args args, Change change, boolean shouldExist, @Nullable RefCache refs) {
+ public ChangeNotes(Args args, Change change, boolean shouldExist, @Nullable RefCache refs) {
super(args, change.getId());
this.change = new Change(change);
this.shouldExist = shouldExist;
@@ -515,10 +510,7 @@
protected void onLoad(LoadHandle handle) throws NoSuchChangeException, IOException {
ObjectId rev = handle.id();
if (rev == null) {
- // TODO(ekempin): Remove the primary storage check. At the moment it is still needed for the
- // ChangeNotesParserTest which still runs with ReviewDb changes (see TODO in
- // TestUpdate#newChange).
- if (PrimaryStorage.of(change) == PrimaryStorage.NOTE_DB && shouldExist) {
+ if (shouldExist) {
throw new NoSuchChangeException(getChangeId());
}
loadDefaults();
diff --git a/java/com/google/gerrit/testing/TestChanges.java b/java/com/google/gerrit/testing/TestChanges.java
index fe4f3e7..a14ca2d 100644
--- a/java/com/google/gerrit/testing/TestChanges.java
+++ b/java/com/google/gerrit/testing/TestChanges.java
@@ -53,7 +53,6 @@
public static Change newChange(Project.NameKey project, Account.Id userId, int id) {
Change.Id changeId = new Change.Id(id);
- // TODO(ekempin): Create NoteDb change.
Change c =
new Change(
new Change.Key("Iabcd1234abcd1234abcd1234abcd1234abcd1234"),
@@ -77,8 +76,8 @@
return ps;
}
- public static ChangeUpdate newUpdate(Injector injector, Change c, CurrentUser user)
- throws Exception {
+ public static ChangeUpdate newUpdate(
+ Injector injector, Change c, CurrentUser user, boolean shouldExist) throws Exception {
injector =
injector.createChildInjector(
new FactoryModule() {
@@ -91,7 +90,9 @@
injector
.getInstance(ChangeUpdate.Factory.class)
.create(
- new ChangeNotes(injector.getInstance(AbstractChangeNotes.Args.class), c).load(),
+ new ChangeNotes(
+ injector.getInstance(AbstractChangeNotes.Args.class), c, shouldExist, null)
+ .load(),
user,
TimeUtil.nowTs(),
Ordering.<String>natural());
diff --git a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
index 138635e..bc35352 100644
--- a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
@@ -192,7 +192,7 @@
protected Change newChange(boolean workInProgress) throws Exception {
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
- ChangeUpdate u = newUpdate(c, changeOwner);
+ ChangeUpdate u = newUpdateForNewChange(c, changeOwner);
u.setChangeId(c.getKey().get());
u.setBranch(c.getDest().get());
u.setWorkInProgress(workInProgress);
@@ -208,15 +208,24 @@
return newChange(false);
}
+ protected ChangeUpdate newUpdateForNewChange(Change c, CurrentUser user) throws Exception {
+ return newUpdate(c, user, false);
+ }
+
protected ChangeUpdate newUpdate(Change c, CurrentUser user) throws Exception {
- ChangeUpdate update = TestChanges.newUpdate(injector, c, user);
+ return newUpdate(c, user, true);
+ }
+
+ protected ChangeUpdate newUpdate(Change c, CurrentUser user, boolean shouldExist)
+ throws Exception {
+ ChangeUpdate update = TestChanges.newUpdate(injector, c, user, shouldExist);
update.setPatchSetId(c.currentPatchSetId());
update.setAllowWriteToNewRef(true);
return update;
}
protected ChangeNotes newNotes(Change c) throws OrmException {
- return new ChangeNotes(args, c).load();
+ return new ChangeNotes(args, c, true, null).load();
}
protected static SubmitRecord submitRecord(
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index e274cdf..033fb12 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -956,7 +956,7 @@
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
String trimmedSubj = c.getSubject();
c.setCurrentPatchSet(c.currentPatchSetId(), " " + trimmedSubj, c.getOriginalSubject());
- ChangeUpdate update = newUpdate(c, changeOwner);
+ ChangeUpdate update = newUpdateForNewChange(c, changeOwner);
update.setChangeId(c.getKey().get());
update.setBranch(c.getDest().get());
update.commit();
@@ -968,7 +968,7 @@
c = TestChanges.newChange(project, changeOwner.getAccountId());
c.setCurrentPatchSet(c.currentPatchSetId(), tabSubj, c.getOriginalSubject());
- update = newUpdate(c, changeOwner);
+ update = newUpdateForNewChange(c, changeOwner);
update.setChangeId(c.getKey().get());
update.setBranch(c.getDest().get());
update.commit();
@@ -3067,7 +3067,7 @@
public void setRevertOfPersistsValue() throws Exception {
Change changeToRevert = newChange();
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
- ChangeUpdate update = newUpdate(c, changeOwner);
+ ChangeUpdate update = newUpdateForNewChange(c, changeOwner);
update.setChangeId(c.getKey().get());
update.setRevertOf(changeToRevert.getId().get());
update.commit();
diff --git a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
index 1b50431..5552572 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
@@ -41,7 +41,7 @@
@Test
public void approvalsCommitFormatSimple() throws Exception {
Change c = TestChanges.newChange(project, changeOwner.getAccountId(), 1);
- ChangeUpdate update = newUpdate(c, changeOwner);
+ ChangeUpdate update = newUpdateForNewChange(c, changeOwner);
update.putApproval("Verified", (short) 1);
update.putApproval("Code-Review", (short) -1);
update.putReviewer(changeOwner.getAccount().getId(), REVIEWER);
@@ -84,7 +84,7 @@
@Test
public void changeMessageCommitFormatSimple() throws Exception {
Change c = TestChanges.newChange(project, changeOwner.getAccountId(), 1);
- ChangeUpdate update = newUpdate(c, changeOwner);
+ ChangeUpdate update = newUpdateForNewChange(c, changeOwner);
update.setChangeMessage("Just a little code change.\nHow about a new line");
update.commit();
assertThat(update.getRefName()).isEqualTo("refs/changes/01/1/meta");
@@ -110,7 +110,7 @@
@Test
public void changeWithRevision() throws Exception {
Change c = TestChanges.newChange(project, changeOwner.getAccountId(), 1);
- ChangeUpdate update = newUpdate(c, changeOwner);
+ ChangeUpdate update = newUpdateForNewChange(c, changeOwner);
update.setChangeMessage("Foo");
RevCommit commit = tr.commit().message("Subject").create();
update.setCommit(rw, commit);
@@ -309,7 +309,7 @@
public void leadingWhitespace() throws Exception {
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
c.setCurrentPatchSet(c.currentPatchSetId(), " " + c.getSubject(), c.getOriginalSubject());
- ChangeUpdate update = newUpdate(c, changeOwner);
+ ChangeUpdate update = newUpdateForNewChange(c, changeOwner);
update.setChangeId(c.getKey().get());
update.setBranch(c.getDest().get());
update.commit();
@@ -330,7 +330,7 @@
c = TestChanges.newChange(project, changeOwner.getAccountId());
c.setCurrentPatchSet(c.currentPatchSetId(), "\t\t" + c.getSubject(), c.getOriginalSubject());
- update = newUpdate(c, changeOwner);
+ update = newUpdateForNewChange(c, changeOwner);
update.setChangeId(c.getKey().get());
update.setBranch(c.getDest().get());
update.commit();
