opendev/gerrit
Code Issues Proposed changes

ChangeRebuilderIT: Reopen ReviewDb when changing migration state

Browse Source 
These tests commonly use methods on TestNotesMigration to play around
with the migration state. However, this wasn't taking into account
the fact that the migration state affects whether a ReviewDb opened
by the SchemaFactory is wrapped in a DisabledReviewDbChangesWrapper.

Unfortunately the request scope implementation used in tests caches
an open ReviewDb, meaning that depending on when the ReviewDb was
first opened, the cached value may be out of sync with the
TestNotesMigration state.

Work around this by always reopening the database, and ensuring the
returned Provider<ReviewDb> caches it as well, whenever the state
changes.

Change-Id: I1bfbbf5492f8232028eba969ace53cc659cb8c8b
This commit is contained in:
Dave Borowitz 2016-05-10 15:06:54 -07:00 committed by Yuxuan 'fishy' Wang
parent 7baf0b546d
commit 96fe10508f
2 changed files with 32 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AcceptanceTestRequestScope.java
View File

@ -182,6 +182,14 @@ public class AcceptanceTestRequestScope {
return old;
}
public Context reopenDb() {
// Setting a new context with the same fields is enough to get the ReviewDb
// provider to reopen the database.
Context old = current.get();
return set(
new Context(old.schemaFactory, old.session, old.user, old.created));
}
/** Returns exactly one instance per command executed. */
static final Scope REQUEST = new Scope() {
@Override

44
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
View File

@ -91,7 +91,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
public void setUp() {
assume().that(NoteDbMode.readWrite()).isFalse();
TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
notesMigration.setAllEnabled(false);
setNotesMigration(false, false);
}
@After
@ -99,6 +99,12 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
TestTimeUtil.useSystemTime();
}
private void setNotesMigration(boolean writeChanges, boolean readChanges) {
notesMigration.setWriteChanges(writeChanges);
notesMigration.setReadChanges(readChanges);
db = atrScope.reopenDb().getReviewDbProvider().get();
}
@Test
public void changeFields() throws Exception {
PushOneCommit.Result r = createChange();
@ -200,8 +206,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
checker.rebuildAndCheckChanges(id);
notesMigration.setWriteChanges(true);
notesMigration.setReadChanges(true);
setNotesMigration(true, true);
ChangeNotes notes = notesFactory.create(db, project, id);
Map<String, PatchSet.Id> psIds = new HashMap<>();
@ -223,7 +228,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
Change.Id id = r.getPatchSetId().getParentKey();
checker.assertNoChangeRef(project, id);
notesMigration.setWriteChanges(true);
setNotesMigration(true, false);
gApi.changes().id(id.get()).topic(name("a-topic"));
// First write doesn't create the ref, but rebuilding works.
@ -251,7 +256,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
public void rebuildViaRestApi() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey();
notesMigration.setWriteChanges(true);
setNotesMigration(true, false);
checker.assertNoChangeRef(project, id);
rebuildHandler.apply(
@ -265,7 +270,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
PushOneCommit.Result r1 = createChange();
Change.Id id1 = r1.getPatchSetId().getParentKey();
notesMigration.setWriteChanges(true);
setNotesMigration(true, false);
gApi.changes().id(id1.get()).topic(name("a-topic"));
PushOneCommit.Result r2 = createChange();
Change.Id id2 = r2.getPatchSetId().getParentKey();
@ -282,7 +287,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
@Test
public void noteDbChangeState() throws Exception {
notesMigration.setAllEnabled(true);
setNotesMigration(true, true);
PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey();
@ -317,20 +322,20 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
@Test
public void rebuildAutomaticallyWhenChangeOutOfDate() throws Exception {
notesMigration.setAllEnabled(true);
setNotesMigration(true, true);
PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey();
assertChangeUpToDate(true, id);
// Make a ReviewDb change behind NoteDb's back and ensure it's detected.
notesMigration.setAllEnabled(false);
setNotesMigration(false, false);
gApi.changes().id(id.get()).topic(name("a-topic"));
setInvalidNoteDbState(id);
assertChangeUpToDate(false, id);
// On next NoteDb read, the change is transparently rebuilt.
notesMigration.setAllEnabled(true);
setNotesMigration(true, true);
assertThat(gApi.changes().id(id.get()).info().topic)
.isEqualTo(name("a-topic"));
assertChangeUpToDate(true, id);
@ -344,7 +349,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
@Test
public void rebuildAutomaticallyWhenDraftsOutOfDate() throws Exception {
notesMigration.setAllEnabled(true);
setNotesMigration(true, true);
setApiUser(user);
PushOneCommit.Result r = createChange();
@ -353,13 +358,13 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
assertDraftsUpToDate(true, id, user);
// Make a ReviewDb change behind NoteDb's back and ensure it's detected.
notesMigration.setAllEnabled(false);
setNotesMigration(false, false);
putDraft(user, id, 1, "comment");
setInvalidNoteDbState(id);
assertDraftsUpToDate(false, id, user);
// On next NoteDb read, the drafts are transparently rebuilt.
notesMigration.setAllEnabled(true);
setNotesMigration(true, true);
assertThat(gApi.changes().id(id.get()).current().drafts())
.containsKey(PushOneCommit.FILE_NAME);
assertDraftsUpToDate(true, id, user);
@ -413,8 +418,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
checker.rebuildAndCheckChanges(id);
notesMigration.setWriteChanges(true);
notesMigration.setReadChanges(true);
setNotesMigration(true, true);
// Rebuild and check was successful, but NoteDb doesn't support storing an
// empty topic, so it comes out as null.
@ -481,7 +485,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
checker.rebuildAndCheckChanges(id);
notesMigration.setAllEnabled(true);
setNotesMigration(true, true);
ChangeNotes notes = notesFactory.create(db, project, id);
Change nc = notes.getChange();
assertThat(nc.getSubject()).isEqualTo(c.getSubject());
@ -504,7 +508,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
checker.rebuildAndCheckChanges(id);
notesMigration.setAllEnabled(true);
setNotesMigration(true, true);
ChangeNotes notes = notesFactory.create(db, project, id);
assertThat(notes.getPatchSets().keySet()).containsExactly(psId);
}
@ -528,7 +532,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
checker.rebuildAndCheckChanges(id);
notesMigration.setAllEnabled(true);
setNotesMigration(true, true);
ChangeNotes notes = notesFactory.create(db, project, id);
assertThat(notes.getComments()).isEmpty();
}
@ -549,7 +553,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
checker.rebuildAndCheckChanges(id);
notesMigration.setAllEnabled(true);
setNotesMigration(true, true);
ChangeNotes notes = notesFactory.create(db, project, id);
assertThat(notes.getPatchSets().keySet())
.containsExactly(change.currentPatchSetId());
@ -568,7 +572,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
checker.rebuildAndCheckChanges(id);
notesMigration.setAllEnabled(true);
setNotesMigration(true, true);
ChangeNotes notes = notesFactory.create(db, project, id);
assertThat(notes.getChange().getSubject()).isNotEqualTo(subj);
assertThat(notes.getChange().getSubject()).isEqualTo(PushOneCommit.SUBJECT);