Skip to content

chore: Add unit tests for options functions and cli#2792

Open
inferrinizzard wants to merge 18 commits into
glideapps:masterfrom
inferrinizzard:chore/bin/cli-tests
Open

chore: Add unit tests for options functions and cli#2792
inferrinizzard wants to merge 18 commits into
glideapps:masterfrom
inferrinizzard:chore/bin/cli-tests

Conversation

@inferrinizzard

@inferrinizzard inferrinizzard commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

Description

Related Issue

#2753

Motivation and Context

Previously testing coverage for CLI functionality was lacking

Previous Behaviour / Output

N/a

New Behaviour / Output

image

How Has This Been Tested?

vitest locally, vitest version ^3.1.4

Screenshots (if appropriate):

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@schani

schani commented Jul 20, 2026

Copy link
Copy Markdown
Member

Thanks for this — splitting src/index.ts into focused modules is a direction we want, and the new unmocked test/cli/options.test.ts is genuinely useful. A few things need fixing before this can merge:

1. Inference flags default to OFF for every invocation (functional bug). In src/inference.ts, inferCLIOptions sets options[flagName] = !opts[cliName] (e.g. inferMaps = !opts.noMaps). Then makeQuicktypeOptions in src/quicktype.options.ts picks up that already-computed options[flagName] as positiveValue and negates it again (quicktypeOptions[flagName] = !value). Net effect: every inference flag (maps, enums, dates, uuids, integer-strings, …) is inverted from what the user asked for, and defaults to disabled. This is exactly what broke branch CI on schema-java: uuid.1.fail.uuid.json stopped failing because UUID inference was silently off, which then fail-fast-cancelled most of the other language jobs.

2. test/cli/quicktype.options.test.ts mocks quicktype-core wholesale, including a hand-rolled inferenceFlagNames: ["inferMaps", "inferEnums"] and a negatedInferenceFlagName mock that produces noInferMaps — not the real convention (noMaps, per negatedInferenceFlagName in src/utils.ts). Because it never exercises the real inferCLIOptionsmakeQuicktypeOptions path, it validated the mock rather than the pipeline, and masked bug 1 completely. Needs rewriting against real flag names/values, unmocked like test/cli/options.test.ts.

3. test/languages.ts: quickTestRendererOptions is narrowed from (RendererOptions | [string, RendererOptions])[] to RendererOptions[], dropping the pinned-input tuple form current master relies on.

4. Real conflicts with current master in 8 files (package.json, package-lock.json, src/GraphQLIntrospection.ts, src/index.ts, test/languages.ts, test/lib/deepEquals.ts, test/lib/multicore.ts, test/test.ts). Master's src/index.ts has since gained fixes (boolean --help type-label fix, int-range-aware number inference, Biome cleanups) that aren't reflected in the new split modules (usage.ts, input.ts/sources.ts) — since this PR relocates that file, resolving means effectively redoing the split on top of current master.

5. This overlaps almost entirely with your other open PR, #2761, which does the same CLI refactor/split. We'd rather not merge both — could you consolidate into a single PR?

Given the overlap and the master drift, would you be up for redoing this as one refactor branched off current master, folding in #2761? The bug-1 fix and the mock-test rewrite are hard requirements, and we'll need a green fixture-test CI run (all languages, not fail-fast-cancelled) before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants