From d1620f06b53572fe10fe22208f5a29a2879d2587 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <git@kchr.de>
Date: Wed, 6 Nov 2024 11:53:20 +0100
Subject: [PATCH] polkadot-service: Fix flaky tests (#6376)
The tests used the same paths. When run on CI, each test is run in its
own process and thus, this "serial_test" crate wasn't used. The tests
are now using their own thread local tempdir, which ensures that the
tests are working when running in parallel in the same program or when
being run individually.
---
Cargo.lock | 26 ------------
Cargo.toml | 1 -
polkadot/node/service/Cargo.toml | 1 -
polkadot/node/service/src/workers.rs | 59 +++++++++++-----------------
4 files changed, 23 insertions(+), 64 deletions(-)
diff --git a/Cargo.lock b/Cargo.lock
index a8e7d7d4cdd..a824faedd6b 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -15992,7 +15992,6 @@ dependencies = [
"sc-transaction-pool-api",
"serde",
"serde_json",
- "serial_test",
"sp-api 26.0.0",
"sp-authority-discovery",
"sp-block-builder",
@@ -20641,31 +20640,6 @@ dependencies = [
"serde",
]
-[[package]]
-name = "serial_test"
-version = "2.0.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "0e56dd856803e253c8f298af3f4d7eb0ae5e23a737252cd90bb4f3b435033b2d"
-dependencies = [
- "dashmap",
- "futures",
- "lazy_static",
- "log",
- "parking_lot 0.12.3",
- "serial_test_derive",
-]
-
-[[package]]
-name = "serial_test_derive"
-version = "2.0.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "91d129178576168c589c9ec973feedf7d3126c01ac2bf08795109aa35b69fb8f"
-dependencies = [
- "proc-macro2 1.0.86",
- "quote 1.0.37",
- "syn 2.0.87",
-]
-
[[package]]
name = "sha-1"
version = "0.9.8"
diff --git a/Cargo.toml b/Cargo.toml
index edfea7b8efa..b12469987ed 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1210,7 +1210,6 @@ serde-big-array = { version = "0.3.2" }
serde_derive = { version = "1.0.117" }
serde_json = { version = "1.0.132", default-features = false }
serde_yaml = { version = "0.9" }
-serial_test = { version = "2.0.0" }
sha1 = { version = "0.10.6" }
sha2 = { version = "0.10.7", default-features = false }
sha3 = { version = "0.10.0", default-features = false }
diff --git a/polkadot/node/service/Cargo.toml b/polkadot/node/service/Cargo.toml
index 3edb3f4dadb..6e8eade21a4 100644
--- a/polkadot/node/service/Cargo.toml
+++ b/polkadot/node/service/Cargo.toml
@@ -140,7 +140,6 @@ polkadot-node-subsystem-test-helpers = { workspace = true }
polkadot-primitives-test-helpers = { workspace = true }
sp-tracing = { workspace = true }
assert_matches = { workspace = true }
-serial_test = { workspace = true }
tempfile = { workspace = true }
[features]
diff --git a/polkadot/node/service/src/workers.rs b/polkadot/node/service/src/workers.rs
index b35bb8302fd..73c3aa46660 100644
--- a/polkadot/node/service/src/workers.rs
+++ b/polkadot/node/service/src/workers.rs
@@ -21,19 +21,20 @@ use is_executable::IsExecutable;
use std::path::PathBuf;
#[cfg(test)]
-use std::sync::{Mutex, OnceLock};
+thread_local! {
+ static TMP_DIR: std::cell::RefCell<Option<tempfile::TempDir>> = std::cell::RefCell::new(None);
+}
/// Override the workers polkadot binary directory path, used for testing.
#[cfg(test)]
-fn workers_exe_path_override() -> &'static Mutex<Option<PathBuf>> {
- static OVERRIDE: OnceLock<Mutex<Option<PathBuf>>> = OnceLock::new();
- OVERRIDE.get_or_init(|| Mutex::new(None))
+fn workers_exe_path_override() -> Option<PathBuf> {
+ TMP_DIR.with_borrow(|t| t.as_ref().map(|t| t.path().join("usr/bin")))
}
+
/// Override the workers lib directory path, used for testing.
#[cfg(test)]
-fn workers_lib_path_override() -> &'static Mutex<Option<PathBuf>> {
- static OVERRIDE: OnceLock<Mutex<Option<PathBuf>>> = OnceLock::new();
- OVERRIDE.get_or_init(|| Mutex::new(None))
+fn workers_lib_path_override() -> Option<PathBuf> {
+ TMP_DIR.with_borrow(|t| t.as_ref().map(|t| t.path().join("usr/lib/polkadot")))
}
/// Determines the final set of paths to use for the PVF workers.
@@ -147,12 +148,9 @@ fn list_workers_paths(
// Consider the /usr/lib/polkadot/ directory.
{
- #[allow(unused_mut)]
- let mut lib_path = PathBuf::from("/usr/lib/polkadot");
+ let lib_path = PathBuf::from("/usr/lib/polkadot");
#[cfg(test)]
- if let Some(ref path_override) = *workers_lib_path_override().lock().unwrap() {
- lib_path = path_override.clone();
- }
+ let lib_path = if let Some(o) = workers_lib_path_override() { o } else { lib_path };
let (prep_worker, exec_worker) = build_worker_paths(lib_path, workers_names);
@@ -175,9 +173,10 @@ fn get_exe_path() -> Result<PathBuf, Error> {
let mut exe_path = std::env::current_exe()?;
let _ = exe_path.pop(); // executable file will always have a parent directory.
#[cfg(test)]
- if let Some(ref path_override) = *workers_exe_path_override().lock().unwrap() {
- exe_path = path_override.clone();
+ if let Some(o) = workers_exe_path_override() {
+ exe_path = o;
}
+
Ok(exe_path)
}
@@ -205,8 +204,7 @@ mod tests {
use super::*;
use assert_matches::assert_matches;
- use serial_test::serial;
- use std::{env::temp_dir, fs, os::unix::fs::PermissionsExt, path::Path};
+ use std::{fs, os::unix::fs::PermissionsExt, path::Path};
const TEST_NODE_VERSION: &'static str = "v0.1.2";
@@ -228,7 +226,7 @@ mod tests {
fn get_program(version: &str) -> String {
format!(
- "#!/bin/bash
+ "#!/usr/bin/env bash
if [[ $# -ne 1 ]] ; then
echo \"unexpected number of arguments: $#\"
@@ -253,27 +251,21 @@ echo {}
) -> Result<(), Box<dyn std::error::Error>> {
// Set up <tmp>/usr/lib/polkadot and <tmp>/usr/bin, both empty.
- let tempdir = temp_dir();
- let lib_path = tempdir.join("usr/lib/polkadot");
- let _ = fs::remove_dir_all(&lib_path);
- fs::create_dir_all(&lib_path)?;
- *workers_lib_path_override().lock()? = Some(lib_path);
+ let tempdir = tempfile::tempdir().unwrap();
+ let tmp_dir = tempdir.path().to_path_buf();
+ TMP_DIR.with_borrow_mut(|t| *t = Some(tempdir));
- let exe_path = tempdir.join("usr/bin");
- let _ = fs::remove_dir_all(&exe_path);
- fs::create_dir_all(&exe_path)?;
- *workers_exe_path_override().lock()? = Some(exe_path.clone());
+ fs::create_dir_all(workers_lib_path_override().unwrap()).unwrap();
+ fs::create_dir_all(workers_exe_path_override().unwrap()).unwrap();
+ let custom_path = tmp_dir.join("usr/local/bin");
// Set up custom path at <tmp>/usr/local/bin.
- let custom_path = tempdir.join("usr/local/bin");
- let _ = fs::remove_dir_all(&custom_path);
- fs::create_dir_all(&custom_path)?;
+ fs::create_dir_all(&custom_path).unwrap();
- f(tempdir, exe_path)
+ f(tmp_dir, workers_exe_path_override().unwrap())
}
#[test]
- #[serial]
fn test_given_worker_path() {
with_temp_dir_structure(|tempdir, exe_path| {
let given_workers_path = tempdir.join("usr/local/bin");
@@ -318,7 +310,6 @@ echo {}
}
#[test]
- #[serial]
fn missing_workers_paths_throws_error() {
with_temp_dir_structure(|tempdir, exe_path| {
// Try with both binaries missing.
@@ -368,7 +359,6 @@ echo {}
}
#[test]
- #[serial]
fn should_find_workers_at_all_locations() {
with_temp_dir_structure(|tempdir, _| {
let prepare_worker_bin_path = tempdir.join("usr/bin/polkadot-prepare-worker");
@@ -394,7 +384,6 @@ echo {}
}
#[test]
- #[serial]
fn should_find_workers_with_custom_names_at_all_locations() {
with_temp_dir_structure(|tempdir, _| {
let (prep_worker_name, exec_worker_name) = ("test-prepare", "test-execute");
@@ -422,7 +411,6 @@ echo {}
}
#[test]
- #[serial]
fn workers_version_mismatch_throws_error() {
let bad_version = "v9.9.9.9";
@@ -474,7 +462,6 @@ echo {}
}
#[test]
- #[serial]
fn should_find_valid_workers() {
// Test bin location.
with_temp_dir_structure(|tempdir, _| {
--
GitLab