From a3f982c1d58a4c96377348aae203ab3f2c234729 Mon Sep 17 00:00:00 2001 From: Matt Mastracci Date: Wed, 28 Feb 2024 09:12:43 -0700 Subject: [PATCH] chore(cli): rename `--trace-ops` to `--trace-leaks` (#22598) As we add tracing to more types of runtime activity, `--trace-ops` is less useful of a name. `--trace-leaks` better reflects that this feature traces both ops and timers, and will eventually trace resource opening as well. This keeps `--trace-ops` as an alias for `--trace-leaks`, but prints a warning to the console suggesting migration to `--trace-leaks`. One test continues to use `--trace-ops` to test the deprecation warning. --------- Signed-off-by: Matt Mastracci --- cli/args/flags.rs | 44 +++++++++++++------ cli/args/mod.rs | 4 +- cli/lsp/testing/execution.rs | 4 +- cli/tools/test/fmt.rs | 10 ++--- cli/tools/test/mod.rs | 8 ++-- tests/integration/test_tests.rs | 6 +-- ...itizer_multiple_timeout_tests_no_trace.out | 4 +- .../test/sanitizer/ops_sanitizer_unstable.out | 2 + 8 files changed, 51 insertions(+), 31 deletions(-) diff --git a/cli/args/flags.rs b/cli/args/flags.rs index 0b74a2f551..a3424ab64e 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -275,7 +275,7 @@ pub struct TestFlags { pub filter: Option, pub shuffle: Option, pub concurrent_jobs: Option, - pub trace_ops: bool, + pub trace_leaks: bool, pub watch: Option, pub reporter: TestReporterConfig, pub junit_path: Option, @@ -2155,7 +2155,14 @@ Directory arguments are expanded to all contained files matching the glob .arg( Arg::new("trace-ops") .long("trace-ops") - .help("Enable tracing of async ops. Useful when debugging leaking ops in test, but impacts test execution time.") + .help("Deprecated alias for --trace-leaks.") + .hide(true) + .action(ArgAction::SetTrue), + ) + .arg( + Arg::new("trace-leaks") + .long("trace-leaks") + .help("Enable tracing of leaks. Useful when debugging leaking ops in test, but impacts test execution time.") .action(ArgAction::SetTrue), ) .arg( @@ -3704,7 +3711,18 @@ fn test_parse(flags: &mut Flags, matches: &mut ArgMatches) { }; let no_run = matches.get_flag("no-run"); - let trace_ops = matches.get_flag("trace-ops"); + let trace_leaks = + matches.get_flag("trace-ops") || matches.get_flag("trace-leaks"); + if trace_leaks && matches.get_flag("trace-ops") { + // We can't change this to use the log crate because its not configured + // yet at this point since the flags haven't been parsed. This flag is + // deprecated though so it's not worth changing the code to use the log + // crate here and this is only done for testing anyway. + eprintln!( + "⚠️ {}", + crate::colors::yellow("The `--trace-ops` flag is deprecated and will be removed in Deno 2.0.\nUse the `--trace-leaks` flag instead."), + ); + } let doc = matches.get_flag("doc"); let allow_none = matches.get_flag("allow-none"); let filter = matches.remove_one::("filter"); @@ -3792,7 +3810,7 @@ fn test_parse(flags: &mut Flags, matches: &mut ArgMatches) { shuffle, allow_none, concurrent_jobs, - trace_ops, + trace_leaks, watch: watch_arg_parse(matches), reporter, junit_path, @@ -7098,7 +7116,7 @@ mod tests { #[test] fn test_with_flags() { #[rustfmt::skip] - let r = flags_from_vec(svec!["deno", "test", "--unstable", "--no-npm", "--no-remote", "--trace-ops", "--no-run", "--filter", "- foo", "--coverage=cov", "--location", "https:foo", "--allow-net", "--allow-none", "dir1/", "dir2/", "--", "arg1", "arg2"]); + let r = flags_from_vec(svec!["deno", "test", "--unstable", "--no-npm", "--no-remote", "--trace-leaks", "--no-run", "--filter", "- foo", "--coverage=cov", "--location", "https:foo", "--allow-net", "--allow-none", "dir1/", "dir2/", "--", "arg1", "arg2"]); assert_eq!( r.unwrap(), Flags { @@ -7114,7 +7132,7 @@ mod tests { }, shuffle: None, concurrent_jobs: None, - trace_ops: true, + trace_leaks: true, coverage_dir: Some("cov".to_string()), watch: Default::default(), reporter: Default::default(), @@ -7196,7 +7214,7 @@ mod tests { ignore: vec![], }, concurrent_jobs: Some(NonZeroUsize::new(4).unwrap()), - trace_ops: false, + trace_leaks: false, coverage_dir: None, watch: Default::default(), junit_path: None, @@ -7229,7 +7247,7 @@ mod tests { ignore: vec![], }, concurrent_jobs: None, - trace_ops: false, + trace_leaks: false, coverage_dir: None, watch: Default::default(), reporter: Default::default(), @@ -7267,7 +7285,7 @@ mod tests { ignore: vec![], }, concurrent_jobs: None, - trace_ops: false, + trace_leaks: false, coverage_dir: None, watch: Default::default(), reporter: Default::default(), @@ -7384,7 +7402,7 @@ mod tests { ignore: vec![], }, concurrent_jobs: None, - trace_ops: false, + trace_leaks: false, coverage_dir: None, watch: Default::default(), reporter: Default::default(), @@ -7415,7 +7433,7 @@ mod tests { ignore: vec![], }, concurrent_jobs: None, - trace_ops: false, + trace_leaks: false, coverage_dir: None, watch: Some(Default::default()), reporter: Default::default(), @@ -7445,7 +7463,7 @@ mod tests { ignore: vec![], }, concurrent_jobs: None, - trace_ops: false, + trace_leaks: false, coverage_dir: None, watch: Some(Default::default()), reporter: Default::default(), @@ -7477,7 +7495,7 @@ mod tests { ignore: vec![], }, concurrent_jobs: None, - trace_ops: false, + trace_leaks: false, coverage_dir: None, watch: Some(WatchFlags { hmr: false, diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 8103f489eb..7fcc56c5f7 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -333,7 +333,7 @@ pub struct TestOptions { pub filter: Option, pub shuffle: Option, pub concurrent_jobs: NonZeroUsize, - pub trace_ops: bool, + pub trace_leaks: bool, pub reporter: TestReporterConfig, pub junit_path: Option, } @@ -361,7 +361,7 @@ impl TestOptions { filter: test_flags.filter, no_run: test_flags.no_run, shuffle: test_flags.shuffle, - trace_ops: test_flags.trace_ops, + trace_leaks: test_flags.trace_leaks, reporter: test_flags.reporter, junit_path: test_flags.junit_path, }) diff --git a/cli/lsp/testing/execution.rs b/cli/lsp/testing/execution.rs index 809d024566..78ee6c7f8c 100644 --- a/cli/lsp/testing/execution.rs +++ b/cli/lsp/testing/execution.rs @@ -296,7 +296,7 @@ impl TestRun { test::TestSpecifierOptions { filter, shuffle: None, - trace_ops: false, + trace_leaks: false, }, )) } @@ -441,7 +441,7 @@ impl TestRun { .iter() .map(|s| s.as_str()), ); - args.push("--trace-ops"); + args.push("--trace-leaks"); if self.workspace_settings.unstable && !args.contains(&"--unstable") { args.push("--unstable"); } diff --git a/cli/tools/test/fmt.rs b/cli/tools/test/fmt.rs index fe2007025c..7dc9ceabf3 100644 --- a/cli/tools/test/fmt.rs +++ b/cli/tools/test/fmt.rs @@ -107,7 +107,7 @@ fn format_sanitizer_accum( } let mut output = vec![]; - let mut needs_trace_ops = false; + let mut needs_trace_leaks = false; for ((item_type, item_name, trace), count) in accum.into_iter() { if item_type == RuntimeActivityType::Resource { let (name, action1, action2) = pretty_resource_name(&item_name); @@ -143,7 +143,7 @@ fn format_sanitizer_accum( value += &if let Some(trace) = trace { format!(" The operation {tense} started here:\n{trace}") } else { - needs_trace_ops = true; + needs_trace_leaks = true; String::new() }; output.push(value); @@ -157,8 +157,8 @@ fn format_sanitizer_accum( ); } } - if needs_trace_ops { - (output, vec!["To get more details where ops were leaked, run again with --trace-ops flag.".to_owned()]) + if needs_trace_leaks { + (output, vec!["To get more details where leaks occurred, run again with the --trace-leaks flag.".to_owned()]) } else { (output, vec![]) } @@ -360,5 +360,5 @@ mod tests { // https://github.com/denoland/deno/issues/13938 leak_format_test!(op_unknown, true, [RuntimeActivity::AsyncOp(0, None, "op_unknown")], " - An async call to op_unknown was started in this test, but never completed.\n\ - To get more details where ops were leaked, run again with --trace-ops flag.\n"); + To get more details where leaks occurred, run again with the --trace-leaks flag.\n"); } diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 526f48c214..13c1c3ed6f 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -440,7 +440,7 @@ struct TestSpecifiersOptions { pub struct TestSpecifierOptions { pub shuffle: Option, pub filter: TestFilter, - pub trace_ops: bool, + pub trace_leaks: bool, } impl TestSummary { @@ -559,7 +559,7 @@ async fn test_specifier_inner( let mut coverage_collector = worker.maybe_setup_coverage_collector().await?; - if options.trace_ops { + if options.trace_leaks { worker.execute_script_static( located_script_name!(), "Deno[Deno.internal].core.setLeakTracingEnabled(true);", @@ -1503,7 +1503,7 @@ pub async fn run_tests( specifier: TestSpecifierOptions { filter: TestFilter::from_flag(&test_options.filter), shuffle: test_options.shuffle, - trace_ops: test_options.trace_ops, + trace_leaks: test_options.trace_leaks, }, }, ) @@ -1647,7 +1647,7 @@ pub async fn run_tests_with_watch( specifier: TestSpecifierOptions { filter: TestFilter::from_flag(&test_options.filter), shuffle: test_options.shuffle, - trace_ops: test_options.trace_ops, + trace_leaks: test_options.trace_leaks, }, }, ) diff --git a/tests/integration/test_tests.rs b/tests/integration/test_tests.rs index f9b47fdbce..cf02872e4c 100644 --- a/tests/integration/test_tests.rs +++ b/tests/integration/test_tests.rs @@ -231,7 +231,7 @@ itest!(ops_sanitizer_timeout_failure { itest!(ops_sanitizer_multiple_timeout_tests { args: - "test --trace-ops test/sanitizer/ops_sanitizer_multiple_timeout_tests.ts", + "test --trace-leaks test/sanitizer/ops_sanitizer_multiple_timeout_tests.ts", exit_code: 1, output: "test/sanitizer/ops_sanitizer_multiple_timeout_tests.out", }); @@ -243,13 +243,13 @@ itest!(ops_sanitizer_multiple_timeout_tests_no_trace { }); itest!(sanitizer_trace_ops_catch_error { - args: "test -A --trace-ops test/sanitizer/trace_ops_caught_error/main.ts", + args: "test -A --trace-leaks test/sanitizer/trace_ops_caught_error/main.ts", exit_code: 0, output: "test/sanitizer/trace_ops_caught_error/main.out", }); itest!(ops_sanitizer_closed_inside_started_before { - args: "test --trace-ops test/sanitizer/ops_sanitizer_closed_inside_started_before.ts", + args: "test --trace-leaks test/sanitizer/ops_sanitizer_closed_inside_started_before.ts", exit_code: 1, output: "test/sanitizer/ops_sanitizer_closed_inside_started_before.out", }); diff --git a/tests/testdata/test/sanitizer/ops_sanitizer_multiple_timeout_tests_no_trace.out b/tests/testdata/test/sanitizer/ops_sanitizer_multiple_timeout_tests_no_trace.out index 0d2863b9ca..3a08089b72 100644 --- a/tests/testdata/test/sanitizer/ops_sanitizer_multiple_timeout_tests_no_trace.out +++ b/tests/testdata/test/sanitizer/ops_sanitizer_multiple_timeout_tests_no_trace.out @@ -8,12 +8,12 @@ test 2 ... FAILED ([WILDCARD]) test 1 => [WILDCARD]/ops_sanitizer_multiple_timeout_tests.ts:[WILDCARD] error: Leaks detected: - 2 async operations to sleep for a duration were started in this test, but never completed. This is often caused by not cancelling a `setTimeout` or `setInterval` call. -To get more details where ops were leaked, run again with --trace-ops flag. +To get more details where leaks occurred, run again with the --trace-leaks flag. test 2 => [WILDCARD]/ops_sanitizer_multiple_timeout_tests.ts:[WILDCARD] error: Leaks detected: - 2 async operations to sleep for a duration were started in this test, but never completed. This is often caused by not cancelling a `setTimeout` or `setInterval` call. -To get more details where ops were leaked, run again with --trace-ops flag. +To get more details where leaks occurred, run again with the --trace-leaks flag. FAILURES diff --git a/tests/testdata/test/sanitizer/ops_sanitizer_unstable.out b/tests/testdata/test/sanitizer/ops_sanitizer_unstable.out index 90990caf5b..1e0fa1d0de 100644 --- a/tests/testdata/test/sanitizer/ops_sanitizer_unstable.out +++ b/tests/testdata/test/sanitizer/ops_sanitizer_unstable.out @@ -1,3 +1,5 @@ +⚠️ The `--trace-ops` flag is deprecated and will be removed in Deno 2.0. +Use the `--trace-leaks` flag instead. Check [WILDCARD]/ops_sanitizer_unstable.ts running 2 tests from [WILDCARD]/ops_sanitizer_unstable.ts no-op ... ok ([WILDCARD])