opendev/gerrit
Code Issues Proposed changes

Remove NotesMigration#enabled()

Browse Source 
The name was too vague to be useful. Many callers assumed it meant reads
were enabled, when in fact it was true when writes were enabled as well.
This caused several tests depending on NoteDb-only features, like
CC-by-email, to fail when GERRIT_NOTEDB=write.

The main non-test usage that was somewhat ambiguous was
AbstractChangeNotes#loadRevision, but it turns out this is only used by
ChangeResource#prepareETag, so using readChanges instead is correct.

Change-Id: I83e3732eb3fc0743860d1fd030526f54a611dc7c
This commit is contained in:
Dave Borowitz
2017-07-13 12:30:31 -04:00
parent 4f65130505
commit e8bad33657
7 changed files with 11 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -1384,7 +1384,7 @@ public class ChangeIT extends AbstractDaemonTest {
@Test
public void addReviewerThatIsInactiveEmailFallback() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
assume().that(notesMigration.readChanges()).isTrue();
ConfigInput conf = new ConfigInput();
conf.enableReviewerByEmail = InheritableBoolean.TRUE;
@@ -1480,7 +1480,7 @@ public class ChangeIT extends AbstractDaemonTest {
@Test
public void addReviewerWithNoteDbWhenDummyApprovalInReviewDbExists() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
assume().that(notesMigration.readChanges()).isTrue();
assume().that(notesMigration.changePrimaryStorage()).isEqualTo(PrimaryStorage.REVIEW_DB);
PushOneCommit.Result r = createChange();
@@ -2159,8 +2159,6 @@ public class ChangeIT extends AbstractDaemonTest {
@Test
public void check() throws Exception {
// TODO(dborowitz): Re-enable when ConsistencyChecker supports NoteDb.
assume().that(notesMigration.enabled()).isFalse();
PushOneCommit.Result r = createChange();
assertThat(gApi.changes().id(r.getChangeId()).get().problems).isNull();
assertThat(gApi.changes().id(r.getChangeId()).get(EnumSet.of(ListChangesOption.CHECK)).problems)

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
View File

@@ -314,7 +314,7 @@ public class ChangeReviewersByEmailIT extends AbstractDaemonTest {
@Test
public void reviewersByEmailAreServedFromIndex() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
assume().that(notesMigration.readChanges()).isTrue();
AccountInfo acc = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com");
for (ReviewerState state : ImmutableList.of(ReviewerState.CC, ReviewerState.REVIEWER)) {

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
View File

@@ -128,7 +128,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
@Test
public void missingRepo() throws Exception {
// NoteDb can't have a change without a repo.
assume().that(notesMigration.enabled()).isFalse();
assume().that(notesMigration.readChanges()).isFalse();
ChangeControl ctl = insertChange();
Project.NameKey name = ctl.getProject().getNameKey();
@@ -141,7 +141,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
public void invalidRevision() throws Exception {
// NoteDb always parses the revision when inserting a patch set, so we can't
// create an invalid patch set.
assume().that(notesMigration.enabled()).isFalse();
assume().that(notesMigration.readChanges()).isFalse();
ChangeControl ctl = insertChange();
PatchSet ps =
@@ -321,7 +321,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
@Test
public void currentPatchSetMissing() throws Exception {
// NoteDb can't create a change without a patch set.
assume().that(notesMigration.enabled()).isFalse();
assume().that(notesMigration.readChanges()).isFalse();
ChangeControl ctl = insertChange();
db.patchSets().deleteKeys(singleton(ctl.getChange().currentPatchSetId()));