Error on conflicting global test directives; fix tests with duplicate directives - #64200
Error on conflicting global test directives; fix tests with duplicate directives#64200Ryan Cavanaugh (RyanCavanaugh) with Copilot wants to merge 3 commits into
Conversation
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
Ryan Cavanaugh (RyanCavanaugh)
left a comment
There was a problem hiding this comment.
Copilot good start but also check that the specified flag value is also legal (barring known back-compat things like module: amd). e.g. You should be able to detect module: commojs or target: es212 as errors
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
Added value legality validation in 3ef8a99 (see commit "Validate legal values for known enum/boolean global test directives"). It checks enum/boolean compiler-option directives (e.g. The check is gated to compiler/transpile test parsing only (new |
There was a problem hiding this comment.
🟡 Changes recommended
The parser has unresolved validation behavior and lacks focused regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Rejects conflicting global test directives and fixes affected compiler fixtures.
Changes:
- Adds duplicate-directive detection and option-value validation.
- Corrects or removes three conflicting directives.
- Updates the CommonJS emit baseline.
Review findings:
- Moderate (2 votes): Remove the unrelated value-validation behavior or document and test it explicitly.
- Moderate (1 vote): Reject empty enum/boolean selectors rather than silently accepting them.
- Nit (2 votes): Add focused tests covering conflicting and identical duplicate directives.
File summaries
| File | Description |
|---|---|
tsc/testdata/tests/cases/conformance/moduleResolution/bundler/bundlerImportTsExtensions.ts |
Removes the duplicate outDir directive. |
tsc/testdata/tests/cases/compiler/useStrictLikePrologueString01.ts |
Corrects target to module. |
tsc/testdata/tests/cases/compiler/es5-yieldFunctionObjectLiterals.ts |
Removes the conflicting lib directive. |
tsc/testdata/baselines/reference/compiler/useStrictLikePrologueString01(target=es2015).js |
Updates the CommonJS emit baseline. |
tsc/internal/testrunner/test_case_parser.go |
Adds duplicate-directive and option-value validation. |
Review details
Suppressed comments (1)
tsc/internal/testrunner/test_case_parser.go:200
- The conflict check compares raw directive text even though the harness normalizes it later. For example,
@declaration: true;and@declaration: trueare semantically identical (trailing semicolons are supported in existing tests), as arees5,es2015andes5, es2015, but this now panics for both pairs. Please compare canonicalized option values here, or reject every duplicate consistently rather than labeling formatting differences as conflicts.
if existingValue, ok := globalOptions[metaDataName]; ok && existingValue != metaDataValue {
panic(fmt.Sprintf("Duplicate global option '%s': %q conflicts with previously set value %q", metaDataName, metaDataValue, existingValue))
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
| func(filename string, content string, fileOptions map[string]string) (*testUnit, error) { | ||
| return &testUnit{content: content, name: filename}, nil | ||
| }, | ||
| ParseTestFilesOptions{ValidateOptionValues: true}, |
| for token := range strings.SplitSeq(value, ",") { | ||
| token = strings.TrimSpace(token) | ||
| if token == "" || token == "*" || strings.HasPrefix(token, "-") || strings.HasPrefix(token, "!") { | ||
| continue | ||
| } | ||
| if !isValid(token) { | ||
| panic(fmt.Sprintf("Illegal value %q for global option '%s'; expected one of: %s", token, name, strings.Join(legalValues(), ", "))) | ||
| } | ||
| } |
| if existingValue, ok := globalOptions[metaDataName]; ok && existingValue != metaDataValue { | ||
| // !!! This would break existing baseline tests | ||
| // panic("Duplicate global option: " + metaDataName) | ||
| panic(fmt.Sprintf("Duplicate global option '%s': %q conflicts with previously set value %q", metaDataName, metaDataValue, existingValue)) |
The Go test harness silently allowed a global test directive (e.g.
@target) to be repeated with a conflicting value—the later value silently won, hiding authoring mistakes.useStrictLikePrologueString01.tshad@target: commonjsimmediately followed by@target: es5, es2015, which almost certainly should have been@module: commonjs, but the harness swallowed the error instead of flagging it.Harness
test_case_parser.go: enabled the (previously disabled/commented-out) check soParseTestFilesAndSymlinksWithOptionsnow panics when a global directive is set twice with different values, instead of silently keeping the last one.Test fixes
Ran the full compiler test suite against the new check to find every existing test that would now trip it (3 total):
useStrictLikePrologueString01.ts:@target: commonjs→@module: commonjs; updated.jsbaseline for the now-active CommonJS emit.es5-yieldFunctionObjectLiterals.ts: removed stray leftover@lib: es5,es2015.promise(overridden by@lib: es2015, likely copy-pasted from an async-function test).bundlerImportTsExtensions.ts: removed stray duplicate@outDir: dist(overridden by@outDir: out).No other baseline changes were needed beyond
useStrictLikePrologueString01.useStrictLikePrologueString01#64198