From 36a63ca4159dc40002cc32d652682848823f0c32 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 28 Apr 2022 12:34:02 -0700 Subject: [PATCH 1/5] many tests do not clean up after themselves --- gateway/tests/integration_tests/serial_console.rs | 7 +++++-- nexus/src/db/datastore.rs | 5 +++++ nexus/src/db/explain.rs | 3 +++ nexus/src/db/lookup.rs | 1 + nexus/tests/integration_tests/updates.rs | 1 + 5 files changed, 15 insertions(+), 2 deletions(-) diff --git a/gateway/tests/integration_tests/serial_console.rs b/gateway/tests/integration_tests/serial_console.rs index 4a32ad110c3..ceda41441a6 100644 --- a/gateway/tests/integration_tests/serial_console.rs +++ b/gateway/tests/integration_tests/serial_console.rs @@ -101,12 +101,13 @@ async fn serial_console_communication() { Message::Binary(msg_from_sp) ); } + + testctx.teardown().await; } #[tokio::test] async fn serial_console_detach() { - let testctx = - setup::test_setup("serial_console_communication", SpPort::One).await; + let testctx = setup::test_setup("serial_console_detach", SpPort::One).await; let client = &testctx.client; let simrack = &testctx.simrack; @@ -185,4 +186,6 @@ async fn serial_console_detach() { ws.next().await.unwrap().unwrap(), Message::Binary(b"world".to_vec()) ); + + testctx.teardown().await; } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 83126f60e72..ce710a3b12b 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -3158,6 +3158,7 @@ mod test { assert_eq!(0, disk1_datasets.intersection(&disk2_datasets).count()); let _ = db.cleanup().await; + logctx.cleanup_successful(); } #[tokio::test] @@ -3224,6 +3225,7 @@ mod test { } let _ = db.cleanup().await; + logctx.cleanup_successful(); } #[tokio::test] @@ -3272,6 +3274,7 @@ mod test { assert!(matches!(err, Error::ServiceUnavailable { .. })); let _ = db.cleanup().await; + logctx.cleanup_successful(); } // TODO: This test should be updated when the correct handling @@ -3318,6 +3321,7 @@ mod test { datastore.region_allocate(&opctx, volume1_id, ¶ms).await.unwrap(); let _ = db.cleanup().await; + logctx.cleanup_successful(); } // Validate that queries which should be executable without a full table @@ -3376,6 +3380,7 @@ mod test { ); let _ = db.cleanup().await; + logctx.cleanup_successful(); } // Test sled-specific IPv6 address allocation diff --git a/nexus/src/db/explain.rs b/nexus/src/db/explain.rs index 3dd5262a83a..c3c1f3a6f13 100644 --- a/nexus/src/db/explain.rs +++ b/nexus/src/db/explain.rs @@ -190,6 +190,7 @@ mod test { ) .await .unwrap(); + logctx.cleanup_successful(); } // Tests the ".explain_async()" method in an asynchronous context. @@ -211,6 +212,7 @@ mod test { .unwrap(); assert_contents("tests/output/test-explain-output", &explanation); + logctx.cleanup_successful(); } // Tests that ".explain()" can tell us when we're doing full table scans. @@ -236,5 +238,6 @@ mod test { "Expected [{}] to contain 'FULL SCAN'", explanation ); + logctx.cleanup_successful(); } } diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index 5bbe68b2cf9..6a3016cf133 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -617,5 +617,6 @@ mod test { } if *o == org_id && **p == project_name)); db.cleanup().await.unwrap(); + logctx.cleanup_successful(); } } diff --git a/nexus/tests/integration_tests/updates.rs b/nexus/tests/integration_tests/updates.rs index b996d91bdba..1bfa25d0a2c 100644 --- a/nexus/tests/integration_tests/updates.rs +++ b/nexus/tests/integration_tests/updates.rs @@ -99,6 +99,7 @@ async fn test_update_end_to_end() { server.close().await.expect("failed to shut down dropshot server"); cptestctx.teardown().await; + logctx.cleanup_successful(); } // =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= From 400cc1c87051f8d0ff0ee1f6e892dd233a7baeca Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 28 Apr 2022 13:17:08 -0700 Subject: [PATCH 2/5] fail CI when successful tests leave stuff in TMPDIR --- .github/buildomat/jobs/build-and-test.sh | 15 +++++++++++++++ .github/workflows/rust.yml | 9 +++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/.github/buildomat/jobs/build-and-test.sh b/.github/buildomat/jobs/build-and-test.sh index 2d204872a94..765d828bb70 100644 --- a/.github/buildomat/jobs/build-and-test.sh +++ b/.github/buildomat/jobs/build-and-test.sh @@ -20,6 +20,13 @@ ptime -m ./tools/ci_download_clickhouse banner cockroach ptime -m bash ./tools/ci_download_cockroachdb +# +# Set up a custom temporary directory within whatever one we were given so that +# we can check later whether we left detritus around. +# +TEST_TMPDIR="${TMPDIR:-/var/tmp}/omicron_tmp" +echo "tests will store output in $TEST_TMPDIR" + # # Put "./cockroachdb/bin" and "./clickhouse" on the PATH for the test # suite. @@ -43,6 +50,7 @@ export PATH="$PATH:$PWD/out/cockroachdb/bin:$PWD/out/clickhouse" banner build export RUSTFLAGS="-D warnings" export RUSTDOCFLAGS="-D warnings" +export TMPDIR=$TEST_TMPDIR ptime -m cargo +'nightly-2021-11-24' build --locked --all-targets --verbose # @@ -57,3 +65,10 @@ ptime -m cargo run --bin omicron-package -- check # banner test ptime -m cargo +'nightly-2021-11-24' test --workspace --locked --verbose + +# +# Make sure that we have left nothing around in $TEST_TMPDIR. The easiest way +# to check is to try to remove it with `rmdir`. +# +unset TMPDIR +rmdir $TEST_TMPDIR diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index ab301e3e129..17fafc128ac 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -145,8 +145,7 @@ jobs: # Put "./cockroachdb/bin" and "./clickhouse" on the PATH for the test # suite. run: TMPDIR=$OMICRON_TMP PATH="$PATH:$PWD/out/cockroachdb/bin:$PWD/out/clickhouse" RUSTFLAGS="-D warnings" RUSTDOCFLAGS="-D warnings" cargo test --no-fail-fast --workspace --locked --verbose - - name: Archive any failed test results - if: ${{ failure() }} + - name: Archive results left by tests # actions/upload-artifact@v2.3.1 uses: actions/upload-artifact@82c141cc518b40d92cc801eee768e7aafc9c2fa2 with: @@ -156,3 +155,9 @@ jobs: ${{ env.OMICRON_TMP }} !${{ env.OMICRON_TMP }}/crdb-base !${{ env.OMICRON_TMP }}/rustc* + # Fail the build if successful tests leave detritus in $TMPDIR. The easiest + # way to check if the directory is empty is to try to remove it with + # `rmdir`. + - name: Remove temporary directory on success (if this fails, tests leaked files in TMPDIR) + if: ${{ success() }} + run: rmdir $OMICRON_TMP From 8b7d0917ff2b39e3706c113db43ac337e6a91126 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 28 Apr 2022 13:21:54 -0700 Subject: [PATCH 3/5] buildomat case could be more explicit --- .github/buildomat/jobs/build-and-test.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/buildomat/jobs/build-and-test.sh b/.github/buildomat/jobs/build-and-test.sh index 765d828bb70..d8608b6ca1c 100644 --- a/.github/buildomat/jobs/build-and-test.sh +++ b/.github/buildomat/jobs/build-and-test.sh @@ -71,4 +71,6 @@ ptime -m cargo +'nightly-2021-11-24' test --workspace --locked --verbose # to check is to try to remove it with `rmdir`. # unset TMPDIR +echo "files in $TEST_TMPDIR (none expected on success):" +find $TEST_TMPDIR -ls rmdir $TEST_TMPDIR From d00eab3da6e2ab371a457f375848f1800647e9e3 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 28 Apr 2022 13:23:20 -0700 Subject: [PATCH 4/5] buildomat: need to create the dir --- .github/buildomat/jobs/build-and-test.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/buildomat/jobs/build-and-test.sh b/.github/buildomat/jobs/build-and-test.sh index d8608b6ca1c..409ad8754ee 100644 --- a/.github/buildomat/jobs/build-and-test.sh +++ b/.github/buildomat/jobs/build-and-test.sh @@ -26,6 +26,7 @@ ptime -m bash ./tools/ci_download_cockroachdb # TEST_TMPDIR="${TMPDIR:-/var/tmp}/omicron_tmp" echo "tests will store output in $TEST_TMPDIR" +mkdir $TEST_TMPDIR # # Put "./cockroachdb/bin" and "./clickhouse" on the PATH for the test From 9670314cda4f12e763a2778a224c58cf1f1a63a6 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 28 Apr 2022 15:28:07 -0700 Subject: [PATCH 5/5] explain tests leak database --- nexus/src/db/explain.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/nexus/src/db/explain.rs b/nexus/src/db/explain.rs index c3c1f3a6f13..d1606b0480c 100644 --- a/nexus/src/db/explain.rs +++ b/nexus/src/db/explain.rs @@ -166,7 +166,7 @@ mod test { #[tokio::test] async fn test_explain() { let logctx = dev::test_setup_log("test_explain"); - let db = test_setup_database(&logctx.log).await; + let mut db = test_setup_database(&logctx.log).await; let cfg = db::Config { url: db.pg_config().clone() }; let pool = db::Pool::new(&cfg); @@ -190,6 +190,7 @@ mod test { ) .await .unwrap(); + db.cleanup().await.unwrap(); logctx.cleanup_successful(); } @@ -197,7 +198,7 @@ mod test { #[tokio::test] async fn test_explain_async() { let logctx = dev::test_setup_log("test_explain_async"); - let db = test_setup_database(&logctx.log).await; + let mut db = test_setup_database(&logctx.log).await; let cfg = db::Config { url: db.pg_config().clone() }; let pool = db::Pool::new(&cfg); @@ -212,6 +213,7 @@ mod test { .unwrap(); assert_contents("tests/output/test-explain-output", &explanation); + db.cleanup().await.unwrap(); logctx.cleanup_successful(); } @@ -219,7 +221,7 @@ mod test { #[tokio::test] async fn test_explain_full_table_scan() { let logctx = dev::test_setup_log("test_explain_full_table_scan"); - let db = test_setup_database(&logctx.log).await; + let mut db = test_setup_database(&logctx.log).await; let cfg = db::Config { url: db.pg_config().clone() }; let pool = db::Pool::new(&cfg); @@ -238,6 +240,7 @@ mod test { "Expected [{}] to contain 'FULL SCAN'", explanation ); + db.cleanup().await.unwrap(); logctx.cleanup_successful(); } }