Skip to content

Commit 507c54a

Browse files
colincaseycolincasey
authored
Fix flaky integration tests (#795)
* Fix flaky integration tests Several of the metrics tests are temporal-based and, while we can get decent reliability by tweaking the timeouts used, it's not a guarantee that the chosen timeouts will always be effective which can lead to unwanted flakiness during test runs or test runs that wait longer than necessary. To balance the tradeoff between wait time and test execution time, this PR introduces `wait_for(condition, timeout)` which will continually poll the state of the given `condition` until it passes or the `timeout` is exceeded. This should help to minimize the wait time required to guarantee the expected outcome. * Fix flaky integration tests Several of the metrics tests are temporal-based and, while we can get decent reliability by tweaking the timeouts used, it's not a guarantee that the chosen timeouts will always be effective which can lead to unwanted flakiness during test runs or test runs that wait longer than necessary. To balance the tradeoff between wait time and test execution time, this PR introduces `wait_for(condition, timeout)` which will continually poll the state of the given `condition` until it passes or the `timeout` is exceeded. This should help to minimize the wait time required to guarantee the expected outcome. * Fix flaky integration tests Several of the metrics tests are temporal-based and, while we can get decent reliability by tweaking the timeouts used, it's not a guarantee that the chosen timeouts will always be effective which can lead to unwanted flakiness during test runs or test runs that wait longer than necessary. To balance the tradeoff between wait time and test execution time, this PR introduces `wait_for(condition, timeout)` which will continually poll the state of the given `condition` until it passes or the `timeout` is exceeded. This should help to minimize the wait time required to guarantee the expected outcome.
1 parent 1144291 commit 507c54a

File tree

2 files changed

+92
-27
lines changed

2 files changed

+92
-27
lines changed

‎buildpacks/nodejs-engine/tests/integration_test.rs

+67-26
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,13 @@ use libcnb_test::{assert_contains, assert_not_contains, ContainerConfig};
55
use std::time::Duration;
66
use test_support::{
77
assert_web_response, nodejs_integration_test, nodejs_integration_test_with_config,
8-
set_node_engine, PORT,
8+
set_node_engine, wait_for, PORT,
99
};
1010

11+
const APPLICATION_STARTUP_TIMEOUT: Duration = Duration::from_secs(5);
12+
const METRICS_SEND_INTERVAL: Duration = Duration::from_secs(10);
13+
const METRICS_SEND_TIMEOUT: Duration = Duration::from_secs(15);
14+
1115
#[test]
1216
#[ignore]
1317
fn simple_indexjs() {
@@ -56,23 +60,43 @@ fn reinstalls_node_if_version_changes() {
5660
fn runtime_metrics_script_is_activated_when_heroku_metrics_url_is_set() {
5761
nodejs_integration_test("./fixtures/node-with-indexjs", |ctx| {
5862
let mut container_config = ContainerConfig::new();
63+
let metrics_send_interval = METRICS_SEND_INTERVAL.as_millis().to_string();
5964
container_config
6065
.expose_port(PORT)
6166
.env("NODE_DEBUG", "heroku")
6267
.env("HEROKU_METRICS_URL", "http://localhost:3000")
63-
.env("METRICS_INTERVAL_OVERRIDE", "10000");
68+
.env("METRICS_INTERVAL_OVERRIDE", &metrics_send_interval);
6469

6570
ctx.start_container(container_config, |container| {
66-
std::thread::sleep(Duration::from_secs(11));
67-
let output = container.logs_now();
68-
assert_contains!(output.stderr, "Registering metrics instrumentation");
69-
assert_contains!(
70-
output.stderr,
71-
"HEROKU_METRICS_URL set to \"http://localhost:3000\""
71+
wait_for(
72+
|| {
73+
let output = container.logs_now();
74+
assert_contains!(output.stderr, "Registering metrics instrumentation");
75+
assert_contains!(
76+
output.stderr,
77+
"HEROKU_METRICS_URL set to \"http://localhost:3000\""
78+
);
79+
assert_contains!(
80+
output.stderr,
81+
&format!("METRICS_INTERVAL_OVERRIDE set to \"{metrics_send_interval}\"")
82+
);
83+
assert_contains!(
84+
output.stderr,
85+
&format!("Using interval of {metrics_send_interval}ms")
86+
);
87+
},
88+
APPLICATION_STARTUP_TIMEOUT,
89+
);
90+
91+
wait_for(
92+
|| {
93+
assert_contains!(
94+
container.logs_now().stderr,
95+
"Sending metrics to http://localhost:3000"
96+
);
97+
},
98+
METRICS_SEND_TIMEOUT,
7299
);
73-
assert_contains!(output.stderr, "METRICS_INTERVAL_OVERRIDE set to \"10000\"");
74-
assert_contains!(output.stderr, "Using interval of 10000ms");
75-
assert_contains!(output.stderr, "Sending metrics to http://localhost:3000");
76100
});
77101
});
78102
}
@@ -87,16 +111,20 @@ fn runtime_metrics_script_is_not_activated_when_heroku_metrics_url_is_not_set()
87111
.env("NODE_DEBUG", "heroku");
88112

89113
ctx.start_container(container_config, |container| {
90-
std::thread::sleep(Duration::from_secs(11));
91-
let output = container.logs_now();
92-
assert_contains!(output.stderr, "Registering metrics instrumentation");
93-
assert_contains!(
94-
output.stderr,
95-
"HEROKU_METRICS_URL was not set in the environment"
96-
);
97-
assert_contains!(
98-
output.stderr,
99-
"Metrics will not be collected for this application"
114+
wait_for(
115+
|| {
116+
let output = container.logs_now();
117+
assert_contains!(output.stderr, "Registering metrics instrumentation");
118+
assert_contains!(
119+
output.stderr,
120+
"HEROKU_METRICS_URL was not set in the environment"
121+
);
122+
assert_contains!(
123+
output.stderr,
124+
"Metrics will not be collected for this application"
125+
);
126+
},
127+
APPLICATION_STARTUP_TIMEOUT,
100128
);
101129
});
102130
});
@@ -120,9 +148,15 @@ fn runtime_metrics_script_is_activated_when_node_version_is_at_least_v14_10_0()
120148
.env("HEROKU_METRICS_URL", "http://localhost:3000");
121149

122150
ctx.start_container(container_config, |container| {
123-
std::thread::sleep(Duration::from_secs(1));
124-
let output = container.logs_now();
125-
assert_contains!(output.stderr, "Registering metrics instrumentation");
151+
wait_for(
152+
|| {
153+
assert_contains!(
154+
container.logs_now().stderr,
155+
"Registering metrics instrumentation"
156+
);
157+
},
158+
APPLICATION_STARTUP_TIMEOUT,
159+
);
126160
});
127161
},
128162
);
@@ -145,8 +179,15 @@ fn runtime_metrics_script_is_not_activated_when_node_version_is_less_than_v14_10
145179
.env("NODE_DEBUG", "heroku");
146180

147181
ctx.start_container(container_config, |container| {
148-
let output = container.logs_now();
149-
assert_not_contains!(output.stderr, "Registering metrics instrumentation");
182+
wait_for(
183+
|| {
184+
assert_not_contains!(
185+
container.logs_now().stderr,
186+
"Registering metrics instrumentation"
187+
);
188+
},
189+
APPLICATION_STARTUP_TIMEOUT,
190+
);
150191
});
151192
},
152193
);

‎test_support/src/lib.rs

+25-1
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,9 @@ use libcnb_test::{
77
TestContext, TestRunner,
88
};
99
use std::net::SocketAddr;
10+
use std::panic;
1011
use std::path::{Path, PathBuf};
11-
use std::time::Duration;
12+
use std::time::{Duration, SystemTime};
1213

1314
const DEFAULT_BUILDER: &str = "heroku/builder:22";
1415
pub const PORT: u16 = 8080;
@@ -119,6 +120,29 @@ pub fn assert_web_response(ctx: &TestContext, expected_response_body: &'static s
119120
});
120121
}
121122

123+
pub fn wait_for<F>(condition: F, max_wait_time: Duration)
124+
where
125+
F: Fn() + panic::RefUnwindSafe,
126+
{
127+
let mut error = None;
128+
let start_time = SystemTime::now();
129+
while SystemTime::now()
130+
.duration_since(start_time)
131+
.expect("should not be an earlier time")
132+
< max_wait_time
133+
{
134+
match panic::catch_unwind(&condition) {
135+
Ok(()) => return,
136+
Err(err) => error = Some(err),
137+
}
138+
std::thread::sleep(Duration::from_millis(10));
139+
}
140+
match error {
141+
None => panic!("timeout exceeded"),
142+
Some(error) => panic::resume_unwind(error),
143+
}
144+
}
145+
122146
pub fn set_node_engine(app_dir: &Path, version_range: &str) {
123147
update_package_json(app_dir, |package_json| {
124148
package_json

0 commit comments

Comments
 (0)