From 63b6c0afa7798e18c33203fa3290ae0254c77ddf Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 16 Jan 2025 17:59:38 +0000 Subject: [PATCH] test: Reorganize backup/restore tests for speed and reliability (#3537) We should do the backup/restore tests _after_ we do the basic tests. This is both more efficient as we avoid an extra up/down cycle and more meaningful as we will back up and restore an actually used system. A bit hard to measure directly as this also moves the initial `docker compose up -w` into the test suite but a random run without this patch took about 10m 49s to finish for the testing part whereas with the patch it came down to 9m 10s so **almost 2 minutes faster**! --- _integration-test/conftest.py | 5 ++++ .../{test_run.py => test_01_basics.py} | 0 .../{test_backup.py => test_02_backup.py} | 30 +++++-------------- action.yaml | 3 +- sentry-admin.sh | 3 +- 5 files changed, 16 insertions(+), 25 deletions(-) rename _integration-test/{test_run.py => test_01_basics.py} (100%) rename _integration-test/{test_backup.py => test_02_backup.py} (84%) diff --git a/_integration-test/conftest.py b/_integration-test/conftest.py index e80cef95e6..e73a7f286c 100644 --- a/_integration-test/conftest.py +++ b/_integration-test/conftest.py @@ -12,6 +12,11 @@ @pytest.fixture(scope="session", autouse=True) def configure_self_hosted_environment(request): + subprocess.run( + ["docker", "compose", "--ansi", "never", "up", "--wait"], + check=True, + capture_output=True, + ) # Create test user subprocess.run( [ diff --git a/_integration-test/test_run.py b/_integration-test/test_01_basics.py similarity index 100% rename from _integration-test/test_run.py rename to _integration-test/test_01_basics.py diff --git a/_integration-test/test_backup.py b/_integration-test/test_02_backup.py similarity index 84% rename from _integration-test/test_backup.py rename to _integration-test/test_02_backup.py index b73e0cfb16..0898f9a951 100644 --- a/_integration-test/test_backup.py +++ b/_integration-test/test_02_backup.py @@ -20,7 +20,7 @@ def test_sentry_admin(setup_backup_restore_env_variables): assert "Usage: ./sentry-admin.sh permissions" in output -def test_backup(setup_backup_restore_env_variables): +def test_01_backup(setup_backup_restore_env_variables): # Docker was giving me permission issues when trying to create this file and write to it even after giving read + write access # to group and owner. Instead, try creating the empty file and then give everyone write access to the backup file file_path = os.path.join(os.getcwd(), "sentry", "backup.json") @@ -41,19 +41,22 @@ def test_backup(setup_backup_restore_env_variables): assert os.path.getsize(file_path) > 0 -def test_import(setup_backup_restore_env_variables): +def test_02_import(setup_backup_restore_env_variables): # Bring postgres down and recreate the docker volume subprocess.run(["docker", "compose", "--ansi", "never", "down"], check=True) # We reset all DB-related volumes here and not just Postgres although the backups # are only for Postgres. The reason is to get a "clean slate" as we need the Kafka - # and Clickhouse volumes to be back to their initial state as well ( without any events) - # We cannot just rm and create them as they still need migrations. + # and Clickhouse volumes to be back to their initial state as well (without any events) + # We cannot just rm and create them as they still need the migrations. for name in ("postgres", "clickhouse", "kafka"): subprocess.run(["docker", "volume", "rm", f"sentry-{name}"], check=True) + subprocess.run(["docker", "volume", "create", f"sentry-{name}"], check=True) subprocess.run( [ "rsync", "-aW", + "--super", + "--numeric-ids", "--no-compress", "--mkpath", join(os.environ["RUNNER_TEMP"], "volumes", f"sentry-{name}", ""), @@ -62,24 +65,6 @@ def test_import(setup_backup_restore_env_variables): check=True, capture_output=True, ) - subprocess.run(["docker", "volume", "create", f"sentry-{name}"], check=True) - - subprocess.run( - [ - "docker", - "run", - "--rm", - "-v", - "sentry-kafka:/data", - "busybox", - "chown", - "-R", - "1000:1000", - "/data", - ], - check=True, - capture_output=True, - ) subprocess.run( ["docker", "compose", "--ansi", "never", "up", "--wait"], @@ -97,3 +82,4 @@ def test_import(setup_backup_restore_env_variables): ], check=True, ) + # TODO: Check something actually restored here like the test user we have from earlier diff --git a/action.yaml b/action.yaml index ec5897f0a2..9f87b93d4f 100644 --- a/action.yaml +++ b/action.yaml @@ -111,13 +111,12 @@ runs: shell: bash run: | sudo chown root /usr/bin/rsync && sudo chmod u+s /usr/bin/rsync - rsync -aW --no-compress --mkpath \ + rsync -aW --super --numeric-ids --no-compress --mkpath \ /var/lib/docker/volumes/sentry-postgres \ /var/lib/docker/volumes/sentry-clickhouse \ /var/lib/docker/volumes/sentry-kafka \ "$RUNNER_TEMP/volumes/" cd ${{ github.action_path }} - docker compose up --wait pytest -x --cov --junitxml=junit.xml _integration-test/ - name: Upload coverage to Codecov diff --git a/sentry-admin.sh b/sentry-admin.sh index 3db5f2ed04..ef0e75e98a 100755 --- a/sentry-admin.sh +++ b/sentry-admin.sh @@ -22,7 +22,8 @@ on the host filesystem. Commands that write files should write them to the '/sen # Actual invocation that runs the command in the container. invocation() { - $dcr --quiet-pull -v "$VOLUME_MAPPING" -T -e SENTRY_LOG_LEVEL=CRITICAL web "$@" 2>&1 + $dc up postgres --wait + $dcr --no-deps --quiet-pull -v "$VOLUME_MAPPING" -T -e SENTRY_LOG_LEVEL=CRITICAL web "$@" 2>&1 } # Function to modify lines starting with `Usage: sentry` to say `Usage: ./sentry-admin.sh` instead.