Accurately recognize fourslash test as submodule #2068
Conversation
| } | ||
|
|
||
| func isSubmoduleTest(testPath string) bool { | ||
| return strings.Contains(testPath, "fourslash/tests/gen") || strings.Contains(testPath, "fourslash/tests/manual") |
There was a problem hiding this comment.
I was worried about this, but runtime.Callers always returns fourward slash paths, so it should work.
There was a problem hiding this comment.
Hadn't really thought about that, but I tested on Windows. 🙂
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the classification of fourslash tests to correctly distinguish between submodule tests (from TypeScript repository) and local tests. Previously, all fourslash tests were treated as submodule tests, but only those in the /gen and /manual subfolders should be. The PR uses runtime caller information to determine the test file path and categorize it appropriately.
Key Changes
- Uses
runtime.Caller(1)to obtain test file path at test creation time - Implements path-based logic to identify submodule tests (those containing
/genor/manualin path) - Updates baseline generation to skip diff creation for non-submodule tests
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/fourslash/fourslash.go |
Captures test path using runtime.Caller and passes it to baseline verification |
internal/fourslash/baselineutil.go |
Adds isSubmoduleTest function and conditionally sets baseline options based on test path |
testdata/baselines/reference/submodule/fourslash/inlayHints/inlayHintsTupleTypeCrash.baseline.diff |
Removes diff baseline for local test |
testdata/baselines/reference/fourslash/inlayHints/inlayHintsTupleTypeCrash.baseline |
Adds proper baseline for local test (non-submodule) |
| func isSubmoduleTest(testPath string) bool { | ||
| return strings.Contains(testPath, "fourslash/tests/gen") || strings.Contains(testPath, "fourslash/tests/manual") | ||
| } |
There was a problem hiding this comment.
The path matching using strings.Contains could produce false positives if 'fourslash/tests/gen' or 'fourslash/tests/manual' appears elsewhere in the absolute path (e.g., in a user's directory name). Consider using filepath.Base(filepath.Dir(testPath)) or a more robust path matching approach like checking if the path ends with these patterns or using path separators for more precise matching.
Noticed in #2040 that we were treating all fourslash tests which we currently diff as submodule tests. This PR makes it so we only treat tests in the
/genand/manualsubfolders as submodule tests, since that's where the ported tests should go.