Skip to content

Commit 73c15a1

Browse files
tercelclaude
andcommitted
fix: use examples/extensions for E2E tests, fix arg access panics
- Replace ./tests/fixtures/extensions with ./examples/extensions in all E2E tests — real modules with module.json + run.sh, no empty fixtures - Remove tests/fixtures/extensions/.gitkeep (no longer needed) - Tighten E2E assertions: describe/exec/math.add now assert exit 0 with real output verification (stdout contains "sum") - Use try_get_one instead of get_one/get_flag in extract_cli_kwargs and reconcile_bool_pairs to avoid panics when schema-derived args don't exist in ArgMatches (e.g. exec subcommand path) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 6f344e8 commit 73c15a1

File tree

3 files changed

+105
-69
lines changed

3 files changed

+105
-69
lines changed

src/cli.rs

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -567,9 +567,21 @@ pub fn reconcile_bool_pairs(
567567
) -> HashMap<String, Value> {
568568
let mut result = HashMap::new();
569569
for pair in bool_pairs {
570-
let pos_set = matches.get_flag(&pair.prop_name);
570+
// Use try_get_one to avoid panicking when the flag doesn't exist
571+
// in ArgMatches (e.g. exec subcommand doesn't have schema-derived flags).
572+
let pos_set = matches
573+
.try_get_one::<bool>(&pair.prop_name)
574+
.ok()
575+
.flatten()
576+
.copied()
577+
.unwrap_or(false);
571578
let neg_id = format!("no-{}", pair.prop_name);
572-
let neg_set = matches.get_flag(&neg_id);
579+
let neg_set = matches
580+
.try_get_one::<bool>(&neg_id)
581+
.ok()
582+
.flatten()
583+
.copied()
584+
.unwrap_or(false);
573585
let val = if pos_set {
574586
true
575587
} else if neg_set {
@@ -606,9 +618,11 @@ fn extract_cli_kwargs(
606618
if id.starts_with("no-") {
607619
continue;
608620
}
609-
if let Some(val) = matches.get_one::<String>(&id) {
621+
// Use try_get_one to avoid panicking when the arg doesn't exist
622+
// in ArgMatches (e.g. exec subcommand doesn't have schema-derived flags).
623+
if let Ok(Some(val)) = matches.try_get_one::<String>(&id) {
610624
kwargs.insert(id, Value::String(val.clone()));
611-
} else if let Some(val) = matches.get_one::<std::path::PathBuf>(&id) {
625+
} else if let Ok(Some(val)) = matches.try_get_one::<std::path::PathBuf>(&id) {
612626
kwargs.insert(id, Value::String(val.to_string_lossy().to_string()));
613627
} else {
614628
kwargs.insert(id, Value::Null);

tests/fixtures/extensions/.gitkeep

Whitespace-only changes.

tests/test_e2e.rs

Lines changed: 87 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@ fn run_apcore(args: &[&str]) -> std::process::Output {
1717

1818
#[test]
1919
fn test_e2e_help_flag_exits_0() {
20-
// `apcore-cli --extensions-dir ./tests/fixtures/extensions --help` must exit 0.
21-
let out = run_apcore(&["--extensions-dir", "./tests/fixtures/extensions", "--help"]);
20+
// `apcore-cli --extensions-dir ./examples/extensions --help` must exit 0.
21+
let out = run_apcore(&["--extensions-dir", "./examples/extensions", "--help"]);
2222
assert_eq!(out.status.code(), Some(0));
2323
}
2424

@@ -34,110 +34,136 @@ fn test_e2e_version_flag() {
3434
#[test]
3535
fn test_e2e_list_command() {
3636
// `apcore-cli --extensions-dir ... list` must exit 0.
37-
let out = run_apcore(&["--extensions-dir", "./tests/fixtures/extensions", "list"]);
37+
let out = run_apcore(&["--extensions-dir", "./examples/extensions", "list"]);
3838
assert_eq!(out.status.code(), Some(0));
3939
}
4040

4141
#[test]
4242
fn test_e2e_describe_command() {
43-
// `apcore-cli --extensions-dir ... describe math.add` must exit 0.
4443
let out = run_apcore(&[
4544
"--extensions-dir",
46-
"./tests/fixtures/extensions",
45+
"./examples/extensions",
4746
"describe",
4847
"math.add",
4948
]);
50-
// Exit 0 once fully implemented; currently exits 0 (stub).
51-
assert!(
52-
out.status.code() == Some(0) || out.status.code() == Some(44),
53-
"describe exits 0 or 44 (stub)"
49+
assert_eq!(
50+
out.status.code(),
51+
Some(0),
52+
"describe math.add must exit 0 with real extensions"
5453
);
5554
}
5655

5756
#[test]
5857
fn test_e2e_execute_math_add() {
59-
// External subcommand "math.add" now routes through dispatch_module.
60-
// With a real extensions dir the module should be found; exit 0 on success
61-
// or 44 if not found in registry (valid module ID format).
58+
// External subcommand "math.add" routes through dispatch_module and
59+
// executes via run.sh with real example extensions.
6260
let out = run_apcore(&[
6361
"--extensions-dir",
64-
"./tests/fixtures/extensions",
62+
"./examples/extensions",
6563
"math.add",
64+
"--a",
65+
"3",
66+
"--b",
67+
"4",
6668
]);
67-
// dispatch_module validates the module ID (exit 2 if invalid format)
68-
// then looks it up in the registry (exit 44 if not found).
69-
// math.add is a valid ID format, so we expect 0 (found) or 44 (not found).
70-
assert!(
71-
out.status.code() == Some(0) || out.status.code() == Some(44),
72-
"math.add via external subcommand must route to dispatch_module, got {:?}",
73-
out.status.code()
69+
assert_eq!(
70+
out.status.code(),
71+
Some(0),
72+
"math.add --a 3 --b 4 must exit 0, got {:?}\nstderr: {}",
73+
out.status.code(),
74+
String::from_utf8_lossy(&out.stderr)
7475
);
75-
// Must NOT contain the old "not yet implemented" message.
76-
let stderr = String::from_utf8_lossy(&out.stderr);
76+
let stdout = String::from_utf8_lossy(&out.stdout);
7777
assert!(
78-
!stderr.contains("not yet implemented"),
79-
"external subcommand must not print 'not yet implemented'"
78+
stdout.contains("\"sum\""),
79+
"output must contain sum field: {stdout}"
8080
);
8181
}
8282

8383
#[test]
8484
fn test_e2e_stdin_piping() {
85-
// External subcommand "math.add --input -" now routes through dispatch_module.
86-
// stdin is /dev/null so collect_input reads empty input.
87-
let out = std::process::Command::new(env!("CARGO_BIN_EXE_apcore-cli"))
85+
// Pipe JSON input via stdin to exec math.add.
86+
use std::io::Write;
87+
let mut child = std::process::Command::new(env!("CARGO_BIN_EXE_apcore-cli"))
8888
.args([
8989
"--extensions-dir",
90-
"./tests/fixtures/extensions",
90+
"./examples/extensions",
9191
"exec",
9292
"math.add",
9393
"--input",
9494
"-",
9595
])
96-
.stdin(std::process::Stdio::null())
97-
.output()
96+
.stdin(std::process::Stdio::piped())
97+
.stdout(std::process::Stdio::piped())
98+
.stderr(std::process::Stdio::piped())
99+
.spawn()
98100
.unwrap();
99-
// dispatch_module validates ID then does registry lookup; expect 0 or 44.
100-
assert!(
101-
out.status.code() == Some(0) || out.status.code() == Some(44),
102-
"exec math.add --input - must route to dispatch_module, got {:?}",
103-
out.status.code()
101+
child
102+
.stdin
103+
.take()
104+
.unwrap()
105+
.write_all(b"{\"a\": 10, \"b\": 20}")
106+
.unwrap();
107+
let out = child.wait_with_output().unwrap();
108+
assert_eq!(
109+
out.status.code(),
110+
Some(0),
111+
"exec math.add --input - must exit 0, stderr: {}",
112+
String::from_utf8_lossy(&out.stderr)
104113
);
105-
let stderr = String::from_utf8_lossy(&out.stderr);
114+
let stdout = String::from_utf8_lossy(&out.stdout);
106115
assert!(
107-
!stderr.contains("not yet implemented"),
108-
"exec subcommand must not print 'not yet implemented'"
116+
stdout.contains("\"sum\""),
117+
"output must contain sum: {stdout}"
109118
);
110119
}
111120

112121
#[test]
113122
fn test_e2e_unknown_module_exits_44() {
114123
let out = run_apcore(&[
115124
"--extensions-dir",
116-
"./tests/fixtures/extensions",
125+
"./examples/extensions",
117126
"nonexistent.module",
118127
]);
119128
assert_eq!(out.status.code(), Some(44));
120129
}
121130

122131
#[test]
123132
fn test_e2e_exec_subcommand_routes_to_dispatch() {
124-
// `apcore-cli exec math.add` must route through dispatch_module.
125-
let out = run_apcore(&[
126-
"--extensions-dir",
127-
"./tests/fixtures/extensions",
128-
"exec",
129-
"math.add",
130-
]);
131-
// Valid module ID format; exit 0 (found) or 44 (not in registry).
132-
assert!(
133-
out.status.code() == Some(0) || out.status.code() == Some(44),
134-
"exec math.add must route to dispatch_module, got {:?}",
135-
out.status.code()
133+
// exec subcommand uses --input - for JSON input (schema flags like --a
134+
// are only available via the external subcommand path, not exec).
135+
use std::io::Write;
136+
let mut child = std::process::Command::new(env!("CARGO_BIN_EXE_apcore-cli"))
137+
.args([
138+
"--extensions-dir",
139+
"./examples/extensions",
140+
"exec",
141+
"math.add",
142+
"--input",
143+
"-",
144+
])
145+
.stdin(std::process::Stdio::piped())
146+
.stdout(std::process::Stdio::piped())
147+
.stderr(std::process::Stdio::piped())
148+
.spawn()
149+
.unwrap();
150+
child
151+
.stdin
152+
.take()
153+
.unwrap()
154+
.write_all(b"{\"a\": 1, \"b\": 2}")
155+
.unwrap();
156+
let out = child.wait_with_output().unwrap();
157+
assert_eq!(
158+
out.status.code(),
159+
Some(0),
160+
"exec math.add must exit 0, stderr: {}",
161+
String::from_utf8_lossy(&out.stderr)
136162
);
137-
let stderr = String::from_utf8_lossy(&out.stderr);
163+
let stdout = String::from_utf8_lossy(&out.stdout);
138164
assert!(
139-
!stderr.contains("not yet implemented"),
140-
"exec subcommand must not print 'not yet implemented'"
165+
stdout.contains("\"sum\""),
166+
"output must contain sum: {stdout}"
141167
);
142168
}
143169

@@ -146,7 +172,7 @@ fn test_e2e_exec_invalid_module_id_exits_2() {
146172
// An invalid module ID format (no dot separator) should exit 2.
147173
let out = run_apcore(&[
148174
"--extensions-dir",
149-
"./tests/fixtures/extensions",
175+
"./examples/extensions",
150176
"exec",
151177
"INVALID",
152178
]);
@@ -161,7 +187,7 @@ fn test_e2e_exec_invalid_module_id_exits_2() {
161187
#[test]
162188
fn test_e2e_external_invalid_module_id_exits_2() {
163189
// An invalid module ID format via external subcommand should exit 2.
164-
let out = run_apcore(&["--extensions-dir", "./tests/fixtures/extensions", "INVALID"]);
190+
let out = run_apcore(&["--extensions-dir", "./examples/extensions", "INVALID"]);
165191
assert_eq!(
166192
out.status.code(),
167193
Some(2),
@@ -173,11 +199,7 @@ fn test_e2e_external_invalid_module_id_exits_2() {
173199
#[test]
174200
fn test_e2e_invalid_input_exits_2() {
175201
// Missing required positional for describe exits 2.
176-
let out = run_apcore(&[
177-
"--extensions-dir",
178-
"./tests/fixtures/extensions",
179-
"describe",
180-
]);
202+
let out = run_apcore(&["--extensions-dir", "./examples/extensions", "describe"]);
181203
assert_eq!(out.status.code(), Some(2));
182204
}
183205

@@ -186,7 +208,7 @@ fn test_e2e_completion_bash() {
186208
// `apcore-cli --extensions-dir ... completion bash` must exit 0.
187209
let out = run_apcore(&[
188210
"--extensions-dir",
189-
"./tests/fixtures/extensions",
211+
"./examples/extensions",
190212
"completion",
191213
"bash",
192214
]);
@@ -199,7 +221,7 @@ fn test_e2e_completion_bash() {
199221

200222
#[test]
201223
fn test_help_flag_exits_0_contains_builtins() {
202-
let out = run_apcore(&["--extensions-dir", "./tests/fixtures/extensions", "--help"]);
224+
let out = run_apcore(&["--extensions-dir", "./examples/extensions", "--help"]);
203225
assert_eq!(out.status.code(), Some(0));
204226
let stdout = String::from_utf8_lossy(&out.stdout);
205227
for builtin in ["list", "describe", "completion"] {
@@ -233,7 +255,7 @@ fn test_extensions_dir_missing_exits_47() {
233255
#[test]
234256
fn test_extensions_dir_env_var_respected() {
235257
let out = std::process::Command::new(env!("CARGO_BIN_EXE_apcore-cli"))
236-
.env("APCORE_EXTENSIONS_ROOT", "./tests/fixtures/extensions")
258+
.env("APCORE_EXTENSIONS_ROOT", "./examples/extensions")
237259
.args(["--help"])
238260
.output()
239261
.unwrap();
@@ -245,7 +267,7 @@ fn test_extensions_dir_flag_overrides_env() {
245267
// --extensions-dir flag takes precedence over APCORE_EXTENSIONS_ROOT.
246268
let out = std::process::Command::new(env!("CARGO_BIN_EXE_apcore-cli"))
247269
.env("APCORE_EXTENSIONS_ROOT", "/nonexistent/path")
248-
.args(["--extensions-dir", "./tests/fixtures/extensions", "--help"])
270+
.args(["--extensions-dir", "./examples/extensions", "--help"])
249271
.output()
250272
.unwrap();
251273
assert_eq!(out.status.code(), Some(0));

0 commit comments

Comments
 (0)