ReplicationTasksStorage: Remove test-only list* methods
Tests should be accurate enough to list from the correct storage areas.
If tests aren't accurate enough when doing so, then the tests should be
improved to provide that accuracy.
The updated ReplicationIT and ReplicationFanoutIT tests should be
investigated in a follow up as tests that currently have an ambiguous
expected state for tasks. For now, they're worked around by considering
both running and waiting tasks. As integration tests, these tests
should be fixed to test behavior, not the specific implementation of
the storage layer.
Partially reverts Ie164b03579cc917f1095cfde6d77cab630f77759.
Change-Id: I021ad40e35e9f1c9d7076bb6984c864743be829f
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
index 36c9c98..048767f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
@@ -83,7 +83,6 @@
private static final Gson GSON = new Gson();
- private final Path refUpdates;
private final Path buildingUpdates;
private final Path runningUpdates;
private final Path waitingUpdates;
@@ -95,7 +94,6 @@
@VisibleForTesting
public ReplicationTasksStorage(Path refUpdates) {
- this.refUpdates = refUpdates;
buildingUpdates = refUpdates.resolve("building");
runningUpdates = refUpdates.resolve("running");
waitingUpdates = refUpdates.resolve("waiting");
@@ -138,21 +136,10 @@
return list(createDir(waitingUpdates));
}
- @VisibleForTesting
public synchronized List<ReplicateRefUpdate> listRunning() {
return list(createDir(runningUpdates));
}
- @VisibleForTesting
- public synchronized List<ReplicateRefUpdate> listBuilding() {
- return list(createDir(buildingUpdates));
- }
-
- @VisibleForTesting
- public synchronized List<ReplicateRefUpdate> list() {
- return list(createDir(refUpdates));
- }
-
private List<ReplicateRefUpdate> list(Path tasks) {
List<ReplicateRefUpdate> results = new ArrayList<>();
try (DirectoryStream<Path> events = Files.newDirectoryStream(tasks)) {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationFanoutIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationFanoutIT.java
index adb7fc8..f06b933 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationFanoutIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationFanoutIT.java
@@ -42,6 +42,7 @@
import java.util.Optional;
import java.util.function.Supplier;
import java.util.regex.Pattern;
+import java.util.stream.Stream;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -108,7 +109,7 @@
input.revision = master;
gApi.projects().name(project.get()).branch(newBranch).create(input);
- assertThat(listReplicationTasks("refs/heads/(mybranch|master)")).hasSize(2);
+ assertThat(listIncompleteTasks("refs/heads/(mybranch|master)")).hasSize(2);
try (Repository repo = repoManager.openRepository(targetProject);
Repository sourceRepo = repoManager.openRepository(project)) {
@@ -134,7 +135,7 @@
RevCommit sourceCommit = pushResult.getCommit();
String sourceRef = pushResult.getPatchSet().refName();
- assertThat(listReplicationTasks("refs/changes/\\d*/\\d*/\\d*")).hasSize(2);
+ assertThat(listIncompleteTasks("refs/changes/\\d*/\\d*/\\d*")).hasSize(2);
try (Repository repo1 = repoManager.openRepository(targetProject1);
Repository repo2 = repoManager.openRepository(targetProject2)) {
@@ -166,7 +167,7 @@
createChange();
- assertThat(listReplicationTasks("refs/changes/\\d*/\\d*/\\d*")).hasSize(4);
+ assertThat(listIncompleteTasks("refs/changes/\\d*/\\d*/\\d*")).hasSize(4);
setReplicationDestination("foo1", replicaSuffixes, ALL_PROJECTS);
setReplicationDestination("foo2", replicaSuffixes, ALL_PROJECTS);
@@ -241,11 +242,15 @@
return projectOperations.newProject().name(name).create();
}
- private List<ReplicateRefUpdate> listReplicationTasks(String refRegex) {
+ @SuppressWarnings(
+ "SynchronizeOnNonFinalField") // tasksStorage is non-final but only set in setUpTestPlugin()
+ private List<ReplicateRefUpdate> listIncompleteTasks(String refRegex) {
Pattern refmaskPattern = Pattern.compile(refRegex);
- return tasksStorage.list().stream()
- .filter(task -> refmaskPattern.matcher(task.ref).matches())
- .collect(toList());
+ synchronized (tasksStorage) {
+ return Stream.concat(tasksStorage.listWaiting().stream(), tasksStorage.listRunning().stream())
+ .filter(task -> refmaskPattern.matcher(task.ref).matches())
+ .collect(toList());
+ }
}
public void cleanupReplicationTasks() throws IOException {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
index 1060780..bfabc9f 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
@@ -105,7 +105,7 @@
Project.NameKey sourceProject = createTestProject("foo");
- assertThat(listReplicationTasks("refs/meta/config")).hasSize(1);
+ assertThat(listIncompleteTasks("refs/meta/config")).hasSize(1);
waitUntil(() -> nonEmptyProjectExists(Project.nameKey(sourceProject + "replica.git")));
@@ -152,7 +152,7 @@
RevCommit sourceCommit = pushResult.getCommit();
String sourceRef = pushResult.getPatchSet().refName();
- assertThat(listReplicationTasks("refs/changes/\\d*/\\d*/\\d*")).hasSize(1);
+ assertThat(listIncompleteTasks("refs/changes/\\d*/\\d*/\\d*")).hasSize(1);
try (Repository repo = repoManager.openRepository(targetProject)) {
waitUntil(() -> checkedGetRef(repo, sourceRef) != null);
@@ -175,7 +175,7 @@
input.revision = master;
gApi.projects().name(project.get()).branch(newBranch).create(input);
- assertThat(listReplicationTasks("refs/heads/(mybranch|master)")).hasSize(2);
+ assertThat(listIncompleteTasks("refs/heads/(mybranch|master)")).hasSize(2);
try (Repository repo = repoManager.openRepository(targetProject);
Repository sourceRepo = repoManager.openRepository(project)) {
@@ -201,7 +201,7 @@
RevCommit sourceCommit = pushResult.getCommit();
String sourceRef = pushResult.getPatchSet().refName();
- assertThat(listReplicationTasks("refs/changes/\\d*/\\d*/\\d*")).hasSize(2);
+ assertThat(listIncompleteTasks("refs/changes/\\d*/\\d*/\\d*")).hasSize(2);
try (Repository repo1 = repoManager.openRepository(targetProject1);
Repository repo2 = repoManager.openRepository(targetProject2)) {
@@ -233,7 +233,7 @@
createChange();
- assertThat(listReplicationTasks("refs/changes/\\d*/\\d*/\\d*")).hasSize(4);
+ assertThat(listIncompleteTasks("refs/changes/\\d*/\\d*/\\d*")).hasSize(4);
setReplicationDestination("foo1", replicaSuffixes, ALL_PROJECTS);
setReplicationDestination("foo2", replicaSuffixes, ALL_PROJECTS);
@@ -251,7 +251,7 @@
.getInstance(ReplicationQueue.class)
.scheduleFullSync(project, null, new ReplicationState(NO_OP), true);
- assertThat(listReplicationTasks(".*all.*")).hasSize(1);
+ assertThat(listIncompleteTasks(".*all.*")).hasSize(1);
}
@Test
@@ -269,8 +269,8 @@
.getInstance(ReplicationQueue.class)
.scheduleFullSync(project, urlMatch, new ReplicationState(NO_OP), true);
- assertThat(listReplicationTasks(".*all.*")).hasSize(1);
- for (ReplicationTasksStorage.ReplicateRefUpdate task : tasksStorage.list()) {
+ assertThat(listIncompleteTasks(".*all.*")).hasSize(1);
+ for (ReplicationTasksStorage.ReplicateRefUpdate task : listIncompleteTasks()) {
assertThat(task.uri).isEqualTo(expectedURI);
}
}
@@ -290,11 +290,10 @@
.getInstance(ReplicationQueue.class)
.scheduleFullSync(project, urlMatch, new ReplicationState(NO_OP), true);
- assertThat(listReplicationTasks(".*")).hasSize(1);
- for (ReplicationTasksStorage.ReplicateRefUpdate task : tasksStorage.list()) {
+ assertThat(listIncompleteTasks()).hasSize(1);
+ for (ReplicationTasksStorage.ReplicateRefUpdate task : listIncompleteTasks()) {
assertThat(task.uri).isEqualTo(expectedURI);
}
- assertThat(tasksStorage.list()).isNotEmpty();
}
@Test
@@ -342,7 +341,7 @@
input.revision = master;
gApi.projects().name(project.get()).branch(branchToDelete).create(input);
- assertThat(listReplicationTasks("refs/heads/(todelete|master)")).hasSize(2);
+ assertThat(listIncompleteTasks("refs/heads/(todelete|master)")).hasSize(2);
try (Repository repo = repoManager.openRepository(targetProject)) {
waitUntil(() -> checkedGetRef(repo, branchToDelete) != null);
@@ -350,7 +349,7 @@
gApi.projects().name(project.get()).branch(branchToDelete).delete();
- assertThat(listReplicationTasks(branchToDelete)).hasSize(1);
+ assertThat(listIncompleteTasks(branchToDelete)).hasSize(1);
try (Repository repo = repoManager.openRepository(targetProject)) {
if (mirror) {
@@ -464,7 +463,7 @@
String changeRef = createChange().getPatchSet().refName();
tasksStorage.disableDeleteForTesting(false);
- changeReplicationTasksForRemote(changeRef, remote1)
+ changeReplicationTasksForRemote(tasksStorage.listWaiting().stream(), changeRef, remote1)
.forEach(
(update) -> {
try {
@@ -601,7 +600,12 @@
private Stream<ReplicateRefUpdate> changeReplicationTasksForRemote(
String changeRef, String remote) {
- return tasksStorage.list().stream()
+ return changeReplicationTasksForRemote(streamIncompleteTasks(), changeRef, remote);
+ }
+
+ private Stream<ReplicateRefUpdate> changeReplicationTasksForRemote(
+ Stream<ReplicateRefUpdate> updates, String changeRef, String remote) {
+ return updates
.filter(task -> changeRef.equals(task.ref))
.filter(task -> remote.equals(task.remote));
}
@@ -610,13 +614,26 @@
return projectOperations.newProject().name(name).create();
}
- private List<ReplicateRefUpdate> listReplicationTasks(String refRegex) {
+ private List<ReplicateRefUpdate> listIncompleteTasks(String refRegex) {
Pattern refmaskPattern = Pattern.compile(refRegex);
- return tasksStorage.list().stream()
+ return streamIncompleteTasks()
.filter(task -> refmaskPattern.matcher(task.ref).matches())
.collect(toList());
}
+ private List<ReplicateRefUpdate> listIncompleteTasks() {
+ return streamIncompleteTasks().collect(toList());
+ }
+
+ @SuppressWarnings(
+ "SynchronizeOnNonFinalField") // tasksStorage is non-final but only set in setUpTestPlugin()
+ private Stream<ReplicateRefUpdate> streamIncompleteTasks() {
+ synchronized (tasksStorage) {
+ return Stream.concat(
+ tasksStorage.listWaiting().stream(), tasksStorage.listRunning().stream());
+ }
+ }
+
public void cleanupReplicationTasks() throws IOException {
cleanupReplicationTasks(storagePath);
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
index d69ace0..211eb34 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
@@ -60,13 +60,14 @@
@Test
public void canListEmptyStorage() throws Exception {
- assertThat(storage.list()).isEmpty();
+ assertThat(storage.listWaiting()).isEmpty();
+ assertThat(storage.listRunning()).isEmpty();
}
@Test
public void canListWaitingUpdate() throws Exception {
storage.create(REF_UPDATE);
- assertContainsExactly(storage.list(), REF_UPDATE);
+ assertContainsExactly(storage.listWaiting(), REF_UPDATE);
}
@Test
@@ -74,24 +75,28 @@
storage.create(REF_UPDATE);
storage.start(uriUpdates);
storage.finish(uriUpdates);
- assertThat(storage.list()).isEmpty();
+ assertThat(storage.listWaiting()).isEmpty();
+ assertThat(storage.listRunning()).isEmpty();
}
@Test
public void instancesOfTheSameStorageHaveTheSameElements() throws Exception {
ReplicationTasksStorage persistedView = new ReplicationTasksStorage(storageSite);
- assertThat(storage.list()).isEmpty();
- assertThat(persistedView.list()).isEmpty();
+ assertThat(storage.listWaiting()).isEmpty();
+ assertThat(persistedView.listWaiting()).isEmpty();
storage.create(REF_UPDATE);
- assertContainsExactly(storage.list(), REF_UPDATE);
- assertContainsExactly(persistedView.list(), REF_UPDATE);
+ assertContainsExactly(storage.listWaiting(), REF_UPDATE);
+ assertContainsExactly(persistedView.listWaiting(), REF_UPDATE);
storage.start(uriUpdates);
+ assertThat(storage.listWaiting()).isEmpty();
+ assertThat(persistedView.listWaiting()).isEmpty();
+
storage.finish(uriUpdates);
- assertThat(storage.list()).isEmpty();
- assertThat(persistedView.list()).isEmpty();
+ assertThat(storage.listRunning()).isEmpty();
+ assertThat(persistedView.listRunning()).isEmpty();
}
@Test
@@ -99,7 +104,7 @@
String key = storage.create(REF_UPDATE);
String secondKey = storage.create(REF_UPDATE);
assertEquals(key, secondKey);
- assertContainsExactly(storage.list(), REF_UPDATE);
+ assertContainsExactly(storage.listWaiting(), REF_UPDATE);
}
@Test
@@ -113,7 +118,7 @@
String keyA = storage.create(REF_UPDATE);
String keyB = storage.create(updateB);
- assertThat(storage.list()).hasSize(2);
+ assertThat(storage.listWaiting()).hasSize(2);
assertNotEquals(keyA, keyB);
}
@@ -132,10 +137,10 @@
storage.start(uriUpdatesB);
storage.finish(uriUpdates);
- assertContainsExactly(storage.list(), updateB);
+ assertContainsExactly(storage.listRunning(), updateB);
storage.finish(uriUpdatesB);
- assertThat(storage.list()).isEmpty();
+ assertThat(storage.listRunning()).isEmpty();
}
@Test
@@ -151,7 +156,7 @@
storage.create(updateB);
storage.create(REF_UPDATE);
storage.create(updateB);
- assertThat(storage.list()).hasSize(2);
+ assertThat(storage.listWaiting()).hasSize(2);
}
@Test
@@ -161,7 +166,7 @@
String keyA = storage.create(refA);
String keyB = storage.create(refB);
- assertThat(storage.list()).hasSize(2);
+ assertThat(storage.listWaiting()).hasSize(2);
assertNotEquals(keyA, keyB);
}
@@ -177,10 +182,10 @@
storage.start(uriUpdatesB);
storage.finish(uriUpdatesA);
- assertContainsExactly(storage.list(), refUpdateB);
+ assertContainsExactly(storage.listRunning(), refUpdateB);
storage.finish(uriUpdatesB);
- assertThat(storage.list()).isEmpty();
+ assertThat(storage.listRunning()).isEmpty();
}
@Test(expected = Test.None.class /* no exception expected */)
@@ -215,7 +220,7 @@
storage.finish(uriUpdates);
storage.finish(uriUpdatesB);
- assertThat(storage.list()).isEmpty();
+ assertThat(storage.listRunning()).isEmpty();
}
private void assertContainsExactly(List<ReplicateRefUpdate> all, ReplicateRefUpdate single) {
