JS-2311 S2187: False positive on Angular-style "environment.<env>.ts" config files - #7815
JS-2311 S2187: False positive on Angular-style "environment.<env>.ts" config files#7815oleksandr-selehenenko-sonarsource wants to merge 9 commits into
Conversation
…s config files
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Ruling ReportNo changes to ruling expected issues in this PR |
…apply it to isTestRelatedFile too
|
francois-mora-sonarsource
left a comment
francois-mora-sonarsource
left a comment
There was a problem hiding this comment.
Nice catch on the FP, and thanks for the thorough path tests. I'd like one scoping change before merge, though.
The exception currently fires on the path shape alone, so any project with environments/environment.test.ts gets it, Angular or not. That matters more than usual here because the path is genuinely ambiguous rather than just coincidental: Angular's convention is environment.<env>.ts, so this FP only arises when an environment happens to be named test/spec/cy — which is also exactly what a real colocated test for an environments/environment.ts module looks like. The pattern alone can't distinguish them.
We already have the machinery to prove it's an Angular project: getDependenciesSanitizePaths(context) in helpers/dependency-manifests/dependencies.ts — the same helper S8783 uses for cypress. It reads dependencies/devDependencies/peerDependencies/optionalDependencies from the closest manifest and is cached.
Since isTestFile/isTestRelatedFile are pure path predicates, I'd keep them pure and pass the signal in as an optional argument defaulting to false:
export function isTestFile(filePath: string, extensions?: string[], isAngularProject = false) {
return testFilePattern(extensions).test(filePath) && !(isAngularProject && isEnvironmentConfigFile(filePath));
}S2187/rule.ts:123 then passes a small shared isAngularProject(context) helper — worth putting next to getReactVersion/getVueVersion so the dependency list isn't duplicated if S2925/S8959/S9162 want the same carve-out later.
The opt-in default also fixes a wider side effect. isTestRelatedFile is called from common/filter/filter-path.ts:110, which does MAIN/TEST classification. When sonar.tests isn't configured, that filename heuristic is the only thing marking these files as TEST — so as written, src/environments/environment.test.ts stops being TEST and becomes MAIN if it's under sonar.sources (or is dropped from analysis otherwise). For a genuine Angular config file that's arguably the right outcome, but for a real colocated test in a non-Angular project it means S2925/S8959/S9162 silently stop applying and every main-code rule starts running on the file. That's a blast radius well beyond the S2187 fix, and worth a Peach baseline check either way. Defaulting the new argument to false leaves that path untouched.
Nit, non-blocking: the boolean gets computed before we know the path even matches environments/environment.*, so the dependency lookup runs on every invocation. It's cached, so almost certainly noise — just flagging it.
…etection Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-config shape Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| create(context: Rule.RuleContext) { | ||
| const { filename, settings } = context; | ||
| if (!isTestFile(filename, settings?.testFileExtensions as string[] | undefined)) { | ||
| if ( |
There was a problem hiding this comment.
rules should not have logic to classify scope of the files. This should go into the test file detection heuristic. Once a file is classified as a test by the heuristic, the rule selection in the linter will discard the test files
… rule test coverage, fix filter-path comment Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…p rules and test-file predicates pure Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review 👍 Approved with suggestions 7 resolved / 8 findingsFixes false positives on Angular-style 💡 Quality: isAngularProject doc says "closest manifest" but scans up to baseDir📄 packages/analysis/src/jsts/rules/helpers/dependency-manifests/dependencies.ts:158-172 📄 packages/analysis/src/jsts/rules/helpers/dependency-manifests/dependencies.ts:78-92 📄 packages/analysis/src/common/filter/filter-path.ts:154-167
Align the doc comment with the aggregated (closest-to-topDir) lookup that getDependencies performs.✅ 7 resolved✅ Bug: Environment-config exclusion missing from isTestRelatedFile
✅ Edge Case: Exclusion is path-agnostic, hiding real environment.spec.ts tests
✅ Performance: isAngularProject runs a manifest lookup for every linted file in S2187
✅ Bug: isTestRelatedFile Angular carve-out is never reached in production
✅ Edge Case: Angular carve-out misses environment.e2e.ts / .mock.ts markers
...and 2 more resolved from earlier reviews 🤖 Prompt for agentsImplementation Status ✅ 1 of 1 objectives coveredOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
# Conflicts: # packages/analysis/src/common/filter/filter-path.ts
…ifest union Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review ✅ Approved 8 resolved / 8 findingsFixes S2187 false positives on Angular-style ✅ 8 resolved✅ Bug: Environment-config exclusion missing from isTestRelatedFile
✅ Edge Case: Exclusion is path-agnostic, hiding real environment.spec.ts tests
✅ Performance: isAngularProject runs a manifest lookup for every linted file in S2187
✅ Bug: isTestRelatedFile Angular carve-out is never reached in production
✅ Edge Case: Angular carve-out misses environment.e2e.ts / .mock.ts markers
...and 3 more resolved from earlier reviews Implementation Status ✅ 1 of 1 objectives covered✅ JS-2311 - 1 of 1 objectives coveredThis PR implements the fix for S2187 false positives on Angular-style environment.test.ts configuration files by carving them out of test path classification in Angular projects. ✅ 1 covered here
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
vdiez
left a comment
vdiez
left a comment
There was a problem hiding this comment.
The scope-classification approach looks sound. I have a few suggestions to make the intent exact and keep the test-marker metadata single-sourced.
| * recognised by `isTestRelatedFile`. | ||
| */ | ||
| const ANGULAR_ENVIRONMENT_CONFIG_PATTERN = | ||
| /(?:^|\/)environments?\/environment\.(?:test|spec|cy|e2e|mock)\.[^/]+$/; |
There was a problem hiding this comment.
These markers duplicate the set in test-file-pattern.ts; this already drifted once when e2e/mock were added. Please keep a single canonical definition by exporting a pure isAngularEnvironmentConfigFileCandidate(filePath) predicate from that helper, building both regexes from shared marker constants, and using it here. The Angular dependency check should remain in this classifier.
| // Angular per-environment config files (`environments/environment.<env>.ts`) match the filename | ||
| // heuristic by coincidence when the environment is named test/spec/cy/e2e/mock. In an Angular | ||
| // project they are production config, not tests, so keep them out of the test rule-selection | ||
| // classification: this drops every Test-scoped rule (S2187/S2925/S8959/S9162) for them without | ||
| // any rule needing its own scope logic. Because the carve-out lives here (rule selection only), | ||
| // the scanner/path-derived file type used for metrics is unaffected. The Angular check is gated | ||
| // behind the cheap path match, and its manifest lookup is cached. |
There was a problem hiding this comment.
This comment treats the Angular dependency as proof that the individual file is production configuration and embeds a rule inventory that can go stale. Describe it as an intentional rule-selection classification and avoid naming its current consumers.
| // Angular per-environment config files (`environments/environment.<env>.ts`) match the filename | |
| // heuristic by coincidence when the environment is named test/spec/cy/e2e/mock. In an Angular | |
| // project they are production config, not tests, so keep them out of the test rule-selection | |
| // classification: this drops every Test-scoped rule (S2187/S2925/S8959/S9162) for them without | |
| // any rule needing its own scope logic. Because the carve-out lives here (rule selection only), | |
| // the scanner/path-derived file type used for metrics is unaffected. The Angular check is gated | |
| // behind the cheap path match, and its manifest lookup is cached. | |
| // Angular environment-config candidates can match the filename heuristic by coincidence. | |
| // When `@angular/core` is visible to the file, keep them MAIN for rule selection so test-scoped | |
| // rules do not run. This affects only rule selection; the scanner/path-derived file type used | |
| // for metrics is unchanged. The Angular check is gated by the cheap path predicate, and the | |
| // underlying manifest lookup is cached. |
| /** | ||
| * Determines whether a file belongs to an Angular project, based on the presence of | ||
| * `@angular/core` in the closest dependency manifest. | ||
| * | ||
| * This is a path-based predicate (no `Rule.RuleContext`), so it can be used both by the | ||
| * scope-classification heuristic in `common/filter/filter-path.ts` — which decides MAIN/TEST | ||
| * before any rule runs — and, if needed, by rules. Keeping the Angular signal in one shared | ||
| * helper avoids duplicating the dependency list, and the underlying lookup is cached. | ||
| * | ||
| * @param filePath normalized absolute path of the file | ||
| * @param topDir normalized absolute directory bounding the upward manifest search (project base dir) | ||
| * @returns true when `@angular/core` is declared in the closest manifest | ||
| */ |
There was a problem hiding this comment.
getDependencies intentionally aggregates every dependency manifest from the closest manifest directory through topDir, rather than inspecting only the closest manifest. Align the documentation with that behavior.
| /** | |
| * Determines whether a file belongs to an Angular project, based on the presence of | |
| * `@angular/core` in the closest dependency manifest. | |
| * | |
| * This is a path-based predicate (no `Rule.RuleContext`), so it can be used both by the | |
| * scope-classification heuristic in `common/filter/filter-path.ts` — which decides MAIN/TEST | |
| * before any rule runs — and, if needed, by rules. Keeping the Angular signal in one shared | |
| * helper avoids duplicating the dependency list, and the underlying lookup is cached. | |
| * | |
| * @param filePath normalized absolute path of the file | |
| * @param topDir normalized absolute directory bounding the upward manifest search (project base dir) | |
| * @returns true when `@angular/core` is declared in the closest manifest | |
| */ | |
| /** | |
| * Determines whether `@angular/core` is declared in the dependency manifests visible to a file. | |
| * | |
| * This path-based predicate (no `Rule.RuleContext`) can be used by the scope-classification | |
| * heuristic in `common/filter/filter-path.ts`, which decides MAIN/TEST before any rule runs. | |
| * Keeping the Angular signal in one shared helper avoids duplicating the dependency list, and the | |
| * underlying lookup is cached. | |
| * | |
| * @param filePath normalized absolute path of the file | |
| * @param topDir normalized absolute directory bounding the upward manifest search (project base dir) | |
| * @returns true when `@angular/core` is declared in any manifest from the closest manifest | |
| * directory through `topDir` | |
| */ |
| // JS-2311: Angular per-environment config files (environments/environment.<env>.ts) match the | ||
| // filename heuristic by coincidence. In an Angular project they are production config, so the | ||
| // heuristic used for rule selection carves them out (proved by a real on-disk package.json | ||
| // declaring @angular/core) — keeping them MAIN so Test-scoped rules do not run on them, while | ||
| // the scanner/path-derived file type used for metrics is untouched. Everywhere else they stay | ||
| // TEST for rule selection. |
There was a problem hiding this comment.
@angular/core establishes the Angular signal, but does not prove the purpose of an individual file; “everywhere else” also overlooks explicit scope configuration. This states the tested behavior more precisely.
| // JS-2311: Angular per-environment config files (environments/environment.<env>.ts) match the | |
| // filename heuristic by coincidence. In an Angular project they are production config, so the | |
| // heuristic used for rule selection carves them out (proved by a real on-disk package.json | |
| // declaring @angular/core) — keeping them MAIN so Test-scoped rules do not run on them, while | |
| // the scanner/path-derived file type used for metrics is untouched. Everywhere else they stay | |
| // TEST for rule selection. | |
| // JS-2311: When sonar.tests is not configured, Angular environment-config filenames can match | |
| // the test-file heuristic by coincidence. If `@angular/core` is visible to the file, keep these | |
| // candidates MAIN for rule selection. The scanner/path-derived file type used for metrics is | |
| // unchanged; non-Angular candidates continue through the normal test-file heuristic. |
| it('respects an explicit TEST classification (sonar.tests) over the Angular carve-out', () => { | ||
| // When the scanner already typed the file as TEST, rule selection keeps it TEST. |
There was a problem hiding this comment.
This test passes TEST directly; it does not configure sonar.tests. Name the behavior it actually exercises.
| it('respects an explicit TEST classification (sonar.tests) over the Angular carve-out', () => { | |
| // When the scanner already typed the file as TEST, rule selection keeps it TEST. | |
| it('preserves an existing TEST classification over the Angular carve-out', () => { | |
| // An upstream TEST classification takes precedence over the filename heuristic. |
3 participants
Part of JS-2311
Summary by Gitar
environments/environment.*.tsconfig files from being flagged as test files byisTestFileandisTestRelatedFile.@angular/coreto restrict the environment config exclusion strictly to Angular projects.This will update automatically on new commits.