Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions generators/cli/changes/unreleased/fix-wire-test-no-pager.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
- summary: |
`generateWireTests: true` no longer emits a suite in which every test
fails. The harness passed `--no-pager` on every invocation, but the CLI
registers that flag only on operations that declare pagination metadata,
so on a spec with no `x-fern-pagination` markers clap rejected the whole
command line with "unexpected argument" — 100% of cases, not a subset.
The flag was also inert there: the pager only spawns under `--page-all`,
which no case passes. Reproduced end to end on the
`query-parameters-openapi:with-wire-tests` fixture (0/2 passing before,
2/2 after); that fixture also declares no pagination, which is why the
breakage was not caught when the registration gate landed.
type: fix
25 changes: 25 additions & 0 deletions generators/cli/src/__test__/wireTests.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,31 @@ describe("renderWireTestHarness", () => {
expect(rust).toContain("match_header_regex(h.name.as_str()");
});

it("passes only flags the SDK registers on every operation", () => {
// A flag the SDK registers conditionally cannot be pushed
// unconditionally here: clap rejects the whole invocation with
// "unexpected argument", so one mismatched flag fails 100% of cases on
// any spec that misses the condition — not one case, the entire suite.
//
// This happened with --no-pager, which the SDK registers only inside
// `if method_has_pagination(...)`. Every generated suite for a spec
// without pagination metadata failed outright, and the seed fixture
// that would have caught it also has no pagination markers.
const rust = renderWireTestHarness({ binaryName: "acme-cli", cases: [searchCase] });
const pushedFlags = [...rust.matchAll(/args\.push\("(--[a-z0-9-]+)"\.to_string\(\)\)/g)].map(
(match) => match[1]
);
Comment on lines +367 to +369

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion

The regex only catches string-literal pushes of the exact shape args.push("--x".to_string()). A flag pushed via format!, a variable, or with different spacing silently escapes the assertion, so the guard could quietly stop guarding. Not blocking, but worth a comment noting the limitation, or broadening to match any --[a-z0-9-]+ literal in the run_case body.

// Registered unconditionally in `commands::build_cli` for every method.
const alwaysRegistered = ["--base-url", "--params", "--json"];
expect(pushedFlags.length).toBeGreaterThan(0);
for (const flag of pushedFlags) {
expect(
alwaysRegistered,
`${flag} is pushed unconditionally; confirm the SDK registers it on every operation`
).toContain(flag);
}
});

it("mirrors the SDK's namespace stutter-elision when resolving command chains", () => {
const rust = renderWireTestHarness({ binaryName: "acme-cli", cases: [searchCase] });
// The harness must replicate `merge_into_path`'s stutter elision so a
Expand Down
6 changes: 5 additions & 1 deletion generators/cli/src/wireTests/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -908,7 +908,11 @@ async fn run_case(id: &str) {
let mut args: Vec<String> = command.chain.clone();
args.push("--base-url".to_string());
args.push(server.uri());
args.push("--no-pager".to_string());
// Deliberately no --no-pager: it is registered only on operations that
// declare pagination metadata, so pushing it unconditionally made clap
// reject every invocation ("unexpected argument '--no-pager'") on any spec
// without pagination markers — the whole suite, not one case. It was also
// inert here: the pager only spawns under --page-all, which no case passes.
Comment on lines +911 to +915

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion

Five lines of post-mortem in every generated wire_test.rs is a lot of noise for consumers who never saw the bug. Consider trimming to one line in the emitted Rust and keeping the full rationale in the changelog/test comment where it already lives.

Suggested change
// Deliberately no --no-pager: it is registered only on operations that
// declare pagination metadata, so pushing it unconditionally made clap
// reject every invocation ("unexpected argument '--no-pager'") on any spec
// without pagination markers — the whole suite, not one case. It was also
// inert here: the pager only spawns under --page-all, which no case passes.
// No --no-pager: the CLI registers it only on paginated operations, and the
// pager only spawns under --page-all, which no case passes.

if !case.params.is_empty() {
// The CLI reads path params off the baked spec by their wire name, which
// can differ from the manifest's (IR-renamed) name — remap those keys so
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.