Avoid @Sandboxed annotations by resetting project states after each test
When tests are annotated with @Sandboxed they get a fresh server instance for the test and side effects on other tests are prevented. Test that don't use @Sandboxed should have no side effects on other tests so that the order in which the tests are executed doesn't matter. There are 3 issues: 1. The usage of @Sandboxed is expensive and slows down the test execution (e.g. see discussion at [1] or commit message of change I2728106fce). 2. Tests that are not annotated with @Sandboxed sometimes do have unintended side effects on other tests. As result of this tests are flaky and we must spend time on investigating the flakyness (see changes Idb4cd71eab, I2728106fce and Iaec0aa9933 for examples where additional tests had to be sandboxed). 3. Some tests that avoid @Sandboxed due to 1. contain extra code to do cleanups in a finally block. It's bad to have this logic in more and more tests. Generally tests that modify the config of All-Project or All-Users (e.g. by changing permissions, capabilities, label definitions etc.) or that create or modify accounts have side effects on other tests. Hence these tests either needed to be sandboxed or they needed to do custom cleanup. This change implements a ProjectResetter that allows to capture the state of projects before a test and then rollback to this state after the test. This is cheaper than sandboxing tests and makes it in general less likely that tests have unintended side effects on each other. By using the ProjectResetter to reset the state of All-Projects and All-Users after each test most @Sandbox annotations can be removed. When resetting project states it can be controlled by ref patterns which branches should be reset. Resetting the project state is done by saving the states of all refs before the test and then resetting all refs to the saved states after the test. Refs that were newly created during the tests are deleted. Some branches in NoteDb represent entities that are cached or indexed. If they are modified by the ProjectResetter the corresponding entities need to be evicted from the caches and reindexed: * If resetting touches refs/meta/config branches the corresponding projects are evicted from the project cache (which triggers a reindex). * If resetting touches user branches or the refs/meta/external-ids branch the corresponding accounts are evicted from the account cache (which triggers a reindex) and also if needed from the cache in AccountCreator. At the moment group branches cannot be reset since this would make the group data between ReviewDb and NoteDb inconsistent. Once groups in ReviewDb are no longer supported we can also reset group branches. There are 2 tests where project resetting currently can't be done: * AbstractNotificationTest has local caching for accounts that should be reused across all test cases. Removing this caching makes this test very slow. To not affect this test resetting project states is disabled for this test. * GroupsIT doesn't reset All-Users since deleting users makes groups inconsistent (e.g. groups would contain members that no longer exist) and as result of this the group consistency checker that is executed after each test would fail. Once groups are only in NoteDb project resetting (including group branches) can be enabled for this test. Removing almost all usages of @Sandboxed makes the tests faster, e.g. for ChangeIT the execution goes down from ~90s [2] to ~35s [3] which is 2.5x speed increase. [1] https://gerrit-review.googlesource.com/c/gerrit/+/142232/2/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java#1068 [2] $ bazel test --test_filter=ChangeIT --runs_per_test=2 //javatests/com/google/gerrit/acceptance/api/change:api_change ... //javatests/com/google/gerrit/acceptance/api/change:api_change PASSED in 91.4s Stats over 2 runs: max = 91.4s, min = 90.1s, avg = 90.7s, dev = 0.7s [3] $ bazel test --test_filter=ChangeIT --runs_per_test=2 //javatests/com/google/gerrit/acceptance/api/change:api_change ... //javatests/com/google/gerrit/acceptance/api/change:api_change PASSED in 36.2s Stats over 2 runs: max = 36.2s, min = 35.5s, avg = 35.9s, dev = 0.4s Change-Id: I1bb46bb18d62a1497447d470c4e96aa859570cd3 Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
@@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableList;
|
||||
import com.google.common.truth.FailureMetadata;
|
||||
import com.google.common.truth.Subject;
|
||||
import com.google.common.truth.Truth;
|
||||
import com.google.gerrit.acceptance.ProjectResetter.Builder;
|
||||
import com.google.gerrit.common.Nullable;
|
||||
import com.google.gerrit.extensions.api.changes.RecipientType;
|
||||
import com.google.gerrit.extensions.api.changes.ReviewInput;
|
||||
@@ -61,6 +62,13 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest {
|
||||
gApi.projects().name(project.get()).config(conf);
|
||||
}
|
||||
|
||||
@Override
|
||||
protected ProjectResetter resetProjects(Builder resetter) throws IOException {
|
||||
// Don't reset anything so that stagedUsers can be cached across all tests.
|
||||
// Without this caching these tests become much too slow.
|
||||
return resetter.build();
|
||||
}
|
||||
|
||||
protected static FakeEmailSenderSubject assertThat(FakeEmailSender sender) {
|
||||
return assertAbout(FakeEmailSenderSubject::new).that(sender);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user