jszwedko
commented
3 years ago For what it's worth, it does look like ntest_timeout works with tokio::test, it just introduces an additional constraint on the return value (if there is one) of Send which was causing the error we were seeing of:
57 | #[timeout(1000)]
| ^^^^^^^^^^^^^^^^ `(dyn std::error::Error + 'static)` cannot be sent between threads safely
|
::: /Users/jszwedko/.cargo/registry/src/github.com-1ecc6299db9ec823/ntest_proc_macro_helper-0.7.3/src/lib.rs:8:32
|
8 | pub fn execute_with_timeout<T: Send>(
| ---- required by this bound in `ntest::ntest_proc_macro_helper::execute_with_timeout`
|
= help: the trait `std::marker::Send` is not implemented for `(dyn std::error::Error + 'static)`
= note: required because of the requirements on the impl of `std::marker::Send` for `Unique<(dyn std::error::Error + 'static)>`
= note: required because it appears within the type `Box<(dyn std::error::Error + 'static)>`
= note: required because it appears within the type `Result<(), Box<(dyn std::error::Error + 'static)>>`
= note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: aborting due to previous errorI was able to workaround the error by not returning a Result from the test function and instead just unwrapping everywhere with this diff:
diff --git a/Cargo.lock b/Cargo.lock
index 70fdc722b..b932cb776 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -3276,6 +3276,7 @@ dependencies = [
"indoc",
"k8s-openapi",
"k8s-test-framework",
+ "ntest",
"rand 0.8.3",
"regex",
"reqwest",
@@ -4302,6 +4303,47 @@ dependencies = [
"winapi 0.3.9",
]
+[[package]]
+name = "ntest"
+version = "0.7.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "984caf6c8aa869418ef88062fc685d07d50c04308e63f7eaff6a395b1f5aff33"
+dependencies = [
+ "ntest_proc_macro_helper",
+ "ntest_test_cases",
+ "ntest_timeout",
+]
+
+[[package]]
+name = "ntest_proc_macro_helper"
+version = "0.7.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "115562228962147ca51748d19446a4261535a7b6a7b5ff02681e527dcefc22f7"
+
+[[package]]
+name = "ntest_test_cases"
+version = "0.7.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c03e3201148714c580c5cf1ef68b1ed4039203068193197834a43503164b8237"
+dependencies = [
+ "proc-macro2 1.0.26",
+ "quote 1.0.9",
+ "syn 1.0.72",
+]
+
+[[package]]
+name = "ntest_timeout"
+version = "0.7.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a870300c30d4224cb16022a4660fd8084d6cb4d29618563c3016b50cc757d1e7"
+dependencies = [
+ "ntest_proc_macro_helper",
+ "proc-macro-crate",
+ "proc-macro2 1.0.26",
+ "quote 1.0.9",
+ "syn 1.0.72",
+]
+
[[package]]
name = "nuid"
version = "0.2.2"
diff --git a/lib/k8s-e2e-tests/Cargo.toml b/lib/k8s-e2e-tests/Cargo.toml
index d82cf16ae..bcad08511 100644
--- a/lib/k8s-e2e-tests/Cargo.toml
+++ b/lib/k8s-e2e-tests/Cargo.toml
@@ -20,6 +20,9 @@ env_logger = "0.8"
tracing = { version = "0.1", features = ["log"] }
rand = "0.8"
+[dev-dependencies]
+ntest = "*"
+
[features]
e2e-tests = []
diff --git a/lib/k8s-e2e-tests/tests/vector-agent.rs b/lib/k8s-e2e-tests/tests/vector-agent.rs
index 86582a8f3..fe482040b 100644
--- a/lib/k8s-e2e-tests/tests/vector-agent.rs
+++ b/lib/k8s-e2e-tests/tests/vector-agent.rs
@@ -4,6 +4,7 @@ use k8s_e2e_tests::*;
use k8s_test_framework::{
lock, test_pod, vector::Config as VectorConfig, wait_for_resource::WaitFor,
};
+use ntest::timeout;
use std::collections::HashSet;
use std::str::FromStr;
use tracing::{debug, info};
@@ -1827,7 +1828,8 @@ async fn host_metrics() -> Result<(), Box<dyn std::error::Error>> {
}
#[tokio::test]
-async fn simple_checkpoint() -> Result<(), Box<dyn std::error::Error>> {
+#[timeout(1000)]
+async fn simple_checkpoint() {
let _guard = lock();
let framework = make_framework();
@@ -1840,28 +1842,34 @@ async fn simple_checkpoint() -> Result<(), Box<dyn std::error::Error>> {
..Default::default()
},
)
- .await?;
+ .await
+ .unwrap();
framework
.wait_for_rollout(
"test-vector",
"daemonset/vector-agent",
vec!["--timeout=60s"],
)
- .await?;
+ .await
+ .unwrap();
- let test_namespace = framework.namespace("test-vector-test-pod").await?;
+ let test_namespace = framework.namespace("test-vector-test-pod").await.unwrap();
let test_pod = framework
- .test_pod(test_pod::Config::from_pod(&make_test_pod(
- "test-vector-test-pod",
- "test-pod",
- // This allows us to read and checkpoint the first log
- // then ensure we just read the new marker after restarting Vector
- "echo CHECKED_MARKER; sleep 60; echo MARKER",
- vec![],
- vec![],
- ))?)
- .await?;
+ .test_pod(
+ test_pod::Config::from_pod(&make_test_pod(
+ "test-vector-test-pod",
+ "test-pod",
+ // This allows us to read and checkpoint the first log
+ // then ensure we just read the new marker after restarting Vector
+ "echo CHECKED_MARKER; sleep 60; echo MARKER",
+ vec![],
+ vec![],
+ ))
+ .unwrap(),
+ )
+ .await
+ .unwrap();
framework
.wait(
"test-vector-test-pod",
@@ -1869,9 +1877,12 @@ async fn simple_checkpoint() -> Result<(), Box<dyn std::error::Error>> {
WaitFor::Condition("initialized"),
vec!["--timeout=60s"],
)
- .await?;
+ .await
+ .unwrap();
- let mut log_reader = framework.logs("test-vector", "daemonset/vector-agent")?;
+ let mut log_reader = framework
+ .logs("test-vector", "daemonset/vector-agent")
+ .unwrap();
smoke_check_first_line(&mut log_reader).await;
// Read the rest of the log lines.
@@ -1898,12 +1909,14 @@ async fn simple_checkpoint() -> Result<(), Box<dyn std::error::Error>> {
// Request to stop the flow.
FlowControlCommand::Terminate
})
- .await?;
+ .await
+ .unwrap();
assert!(got_marker);
framework
.restart_rollout("test-vector", "daemonset/vector-agent", vec![])
- .await?;
+ .await
+ .unwrap();
// We need to wait for the new pod to start
framework
.wait_for_rollout(
@@ -1911,10 +1924,13 @@ async fn simple_checkpoint() -> Result<(), Box<dyn std::error::Error>> {
"daemonset/vector-agent",
vec!["--timeout=60s"],
)
- .await?;
+ .await
+ .unwrap();
got_marker = false;
// We need to start reading from the newly started pod
- let mut log_reader = framework.logs("test-vector", "daemonset/vector-agent")?;
+ let mut log_reader = framework
+ .logs("test-vector", "daemonset/vector-agent")
+ .unwrap();
look_for_log_line(&mut log_reader, |val| {
if val["kubernetes"]["pod_namespace"] != "test-vector-test-pod" {
return FlowControlCommand::GoOn;
@@ -1938,12 +1954,12 @@ async fn simple_checkpoint() -> Result<(), Box<dyn std::error::Error>> {
// Request to stop the flow.
FlowControlCommand::Terminate
})
- .await?;
+ .await
+ .unwrap();
assert!(got_marker);
drop(test_pod);
drop(test_namespace);
drop(vector);
- Ok(())
}
Related: #7798
While updating our CI version coverage I ran into issues caused by "unexepected" log messages, in this case seemingly generated by
cri-o. Specificallysmoke_check_first_lineseems to be brittle and generally unneeded, which is in place to catch when "things have gone very wrong", which seems like it should just be caught by the tests themselves.It seems easy enough to look for the log we want, rather than erroring on the logs we don't expect - but that creates issues where tests can run without end (aside from CI timeouts). I looked a little bit at adding timeouts which were difficult to implement (personally), and existing tests that have "timeouts" are more based around the number of logs we receive (which also appear much to high based on my initial testing).
My brief research has found little support for timing out tests (tokio in particular), so we may need to do some more work there.