Skip to content
22 changes: 20 additions & 2 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ jobs:
run: cargo doc

build-and-test:
env:
OMICRON_TMP: /tmp/omicron_tmp
needs: skip_duplicate_jobs
if: ${{ needs.skip_duplicate_jobs.outputs.should_skip != 'true' }}
runs-on: ${{ matrix.os }}
Expand Down Expand Up @@ -114,6 +116,8 @@ jobs:
- name: Download CockroachDB binary
if: steps.cache-cockroachdb.outputs.cache-hit != 'true'
run: bash ./tools/ci_download_cockroachdb
- name: Create temporary directory for test outputs
run: mkdir -p $OMICRON_TMP
- name: Build
# We build with:
# - RUSTFLAGS="-D warnings" RUSTDOCFLAGS="-D warnings": disallow warnings
Expand All @@ -125,10 +129,24 @@ jobs:
# also gives us a record of which dependencies were used for each CI
# run. Building with `--locked` ensures that the checked-in Cargo.lock
# is up to date.
run: PATH="$PATH:$PWD/cockroachdb/bin:$PWD/clickhouse" RUSTFLAGS="-D warnings" RUSTDOCFLAGS="-D warnings" cargo build --locked --all-targets --verbose
# - TMPDIR=$OMICRON_TMP: we specify a specific temporary directory so that
# failed test outputs will be in a known place that we can grab at the
# end without also grabbing random other temporary files.
run: TMPDIR=$OMICRON_TMP PATH="$PATH:$PWD/cockroachdb/bin:$PWD/clickhouse" RUSTFLAGS="-D warnings" RUSTDOCFLAGS="-D warnings" cargo build --locked --all-targets --verbose
- name: Run tests
# Use the same RUSTFLAGS and RUSTDOCFLAGS as above to avoid having to
# rebuild here.
# Put "./cockroachdb/bin" and "./clickhouse" on the PATH for the test
# suite.
run: PATH="$PATH:$PWD/cockroachdb/bin:$PWD/clickhouse" RUSTFLAGS="-D warnings" RUSTDOCFLAGS="-D warnings" cargo test --workspace --locked --verbose
run: TMPDIR=$OMICRON_TMP PATH="$PATH:$PWD/cockroachdb/bin:$PWD/clickhouse" RUSTFLAGS="-D warnings" RUSTDOCFLAGS="-D warnings" cargo test --workspace --locked --verbose
- name: Archive any failed test results
if: ${{ failure() }}
# actions/upload-artifact@v2.3.1
uses: actions/upload-artifact@82c141cc518b40d92cc801eee768e7aafc9c2fa2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why the sha rather than a version (which requires a comment) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how well-founded it is, but following the advice in https://julienrenaux.fr/2019/12/20/github-actions-security-risk/.

with:
name: failed_test_outputs_${{ runner.os }}
retention-days: 7
path: |
${{ env.OMICRON_TMP }}
!${{ env.OMICRON_TMP }}/crdb-base
!${{ env.OMICRON_TMP }}/rustc*
2 changes: 1 addition & 1 deletion nexus/src/authz/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ mod test {

#[tokio::test]
async fn test_organization() {
let logctx = dev::test_setup_log("test_database");
let logctx = dev::test_setup_log("test_organization");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated typo in the log file name that I found while doing this.

let mut db = dev::test_setup_database(&logctx.log).await;
let (opctx, datastore) =
crate::db::datastore::datastore_test(&logctx, &db).await;
Expand Down
16 changes: 13 additions & 3 deletions test-utils/src/dev/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,14 @@ impl Drop for CockroachInstance {
}
#[allow(unused_must_use)]
if let Some(temp_dir) = self.temp_dir.take() {
temp_dir.close();
/*
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mentioned in the PR comments: we now deliberately save the database directory if you don't explicitly clean it up after a successful test.

* Do NOT clean up the temporary directory in this case.
*/
let path = temp_dir.into_path();
eprintln!(
"WARN: temporary directory leaked: {}",
path.display()
);
}
}
}
Expand Down Expand Up @@ -1239,13 +1246,13 @@ mod test {
*/
#[tokio::test]
async fn test_database_concurrent() {
let db1 = new_builder()
let mut db1 = new_builder()
.build()
.expect("failed to create starter for the first database")
.start()
.await
.expect("failed to start first database");
let db2 = new_builder()
let mut db2 = new_builder()
.build()
.expect("failed to create starter for the second database")
.start()
Expand Down Expand Up @@ -1274,6 +1281,9 @@ mod test {
client2.query("SELECT v FROM foo", &[]).await.expect("list rows");
assert_eq!(rows.len(), 0);
client2.cleanup().await.expect("second connection closed ungracefully");

db1.cleanup().await.expect("failed to clean up first database");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests always should have been cleaning up the database instead of relying on the best-effort Drop impl. Now that the Drop impl doesn't clean these up any more, they were leaking the directory without this fix.

db2.cleanup().await.expect("failed to clean up second database");
}

/* Success case for make_pg_config() */
Expand Down