Skip to content

Conversation

gabritto
Copy link
Member

This PR ports the fourslash rename tests.
Note that unlike the existing baseline tests, we also diff the rename baseline tests vs the Strada baselines. Once we have user preferences working, we can go look at the remaining diffs to look for bugs/missing features in rename.

The PR also fixes some bugs and differences in fourslash baselines in Corsa compared to Strada, fixes some problems in the conversion script, and removes some code from baseline generation that were unused and I think won't be needed in Corsa (e.g. context spans <| ... |>). If it turns out we need them in the future, I'll add it back.

One key difference in the baselines is that in Corsa, each LS command gets its own baseline file. In Strada, all baselines for a test were written to the same file. I'm keeping that difference here hoping it will make it easier to look at baselines/diffs to see if things are correct.

@gabritto gabritto marked this pull request as ready for review September 11, 2025 21:29
@Copilot Copilot AI review requested due to automatic review settings September 11, 2025 21:29
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ports the fourslash rename tests from the TypeScript submodule to the Go test harness. It introduces baseline testing infrastructure for rename operations and includes fixes for baseline generation and test parsing.

  • Ports rename tests with baseline comparison against TypeScript (Strada) output
  • Implements rename test verification methods for success/failure cases and baseline generation
  • Fixes fourslash test parsing and baseline generation infrastructure
  • Updates conversion script to handle rename-specific test patterns

Reviewed Changes

Copilot reviewed 170 out of 1632 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
internal/fourslash/tests/manual/renameForDefaultExport01_test.go Manual test for default export rename with comment filtering
internal/fourslash/tests/gen/*.go Generated rename test files covering various scenarios (JSX, strings, imports, etc.)
internal/fourslash/test_parser.go Fixes RangeMarker structure and adds stable sorting for ranges
internal/fourslash/fourslash.go Adds rename verification methods and baseline infrastructure
internal/fourslash/baselineutil.go Refactors baseline generation to support multiple commands and rename-specific formatting
internal/fourslash/_scripts/*.mts Updates conversion scripts to handle rename test patterns and preferences

@jakebailey
Copy link
Member

Some of the output filenames have spaces in them; is that intentional? (I can't comment on them directly, github.dev won't let me)

@gabritto
Copy link
Member Author

Some of the output filenames have spaces in them; is that intentional? (I can't comment on them directly, github.dev won't let me)

Ah yes. I think that's because I'm reusing command names (e.g. "Auto Imports") in the baseline file path. Is that likely to cause problems? I can normalize those if so.

@jakebailey
Copy link
Member

Well, it's sort of annoying to write those names out in the terminal, but it's not the worst. So, it's okay.

@iisaduan
Copy link
Member

If not difficult and especially if it's just Auto Imports, I'd prefer the file/folder names normalized for consistency. It's always bothered me that it's one of the only ones (if not the only one) with a space in it when the command is printed in the baseline.

@jakebailey
Copy link
Member

Test failure seems to be a logical race in build mode.

diff --git a/testdata/baselines/reference/tsbuild/sample/explainFiles.js b/testdata/baselines/reference/tsbuild/sample/explainFiles.js
index fa3112b1..ff348ab6 100644
--- a/testdata/baselines/reference/tsbuild/sample/explainFiles.js
+++ b/testdata/baselines/reference/tsbuild/sample/explainFiles.js
@@ -527,7 +527,7 @@ core/index.d.ts
    Imported via '../core/index' from file 'tests/index.ts'
    File is output of project reference source 'core/index.ts'
 core/anotherModule.d.ts
-   Imported via '../core/anotherModule' from file 'tests/index.ts'
+   Imported via '../core/anotherModule' from file 'logic/index.d.ts'
    File is output of project reference source 'core/anotherModule.ts'
 logic/index.d.ts
    Imported via '../logic/index' from file 'tests/index.ts'

@gabritto
Copy link
Member Author

Test failure seems to be a logical race in build mode.

diff --git a/testdata/baselines/reference/tsbuild/sample/explainFiles.js b/testdata/baselines/reference/tsbuild/sample/explainFiles.js
index fa3112b1..ff348ab6 100644
--- a/testdata/baselines/reference/tsbuild/sample/explainFiles.js
+++ b/testdata/baselines/reference/tsbuild/sample/explainFiles.js
@@ -527,7 +527,7 @@ core/index.d.ts
    Imported via '../core/index' from file 'tests/index.ts'
    File is output of project reference source 'core/index.ts'
 core/anotherModule.d.ts
-   Imported via '../core/anotherModule' from file 'tests/index.ts'
+   Imported via '../core/anotherModule' from file 'logic/index.d.ts'
    File is output of project reference source 'core/anotherModule.ts'
 logic/index.d.ts
    Imported via '../logic/index' from file 'tests/index.ts'

@sheetalkamat have you seen this problem before?

@gabritto
Copy link
Member Author

Test failure seems to be a logical race in build mode.

diff --git a/testdata/baselines/reference/tsbuild/sample/explainFiles.js b/testdata/baselines/reference/tsbuild/sample/explainFiles.js
index fa3112b1..ff348ab6 100644
--- a/testdata/baselines/reference/tsbuild/sample/explainFiles.js
+++ b/testdata/baselines/reference/tsbuild/sample/explainFiles.js
@@ -527,7 +527,7 @@ core/index.d.ts
    Imported via '../core/index' from file 'tests/index.ts'
    File is output of project reference source 'core/index.ts'
 core/anotherModule.d.ts
-   Imported via '../core/anotherModule' from file 'tests/index.ts'
+   Imported via '../core/anotherModule' from file 'logic/index.d.ts'
    File is output of project reference source 'core/anotherModule.ts'
 logic/index.d.ts
    Imported via '../logic/index' from file 'tests/index.ts'

@jakebailey I don't think this has anything to do with this PR, right? Since it only touches fourslash test code.

@sheetalkamat
Copy link
Member

@gabritto sorry seeing this and noticing it now .. will look into it. its unrelated to your changes. it should have shown both entries, you can ignore this failure and i will work on fix for it.

@gabritto gabritto added this pull request to the merge queue Sep 12, 2025
Merged via the queue into main with commit 9275bc8 Sep 12, 2025
22 checks passed
@gabritto gabritto deleted the gabritto/fourslash-rename branch September 12, 2025 21:47
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.

5 participants