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>
e748296764