-
Notifications
You must be signed in to change notification settings - Fork 665
Adding manual tests list #1446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding manual tests list #1446
Conversation
There was a problem hiding this 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 adds functionality to manually manage fourslash tests by creating a script that moves generated tests to a separate manual directory and tracks them to prevent regeneration. The purpose is to allow developers to edit auto-generated tests without losing changes when the conversion script runs again.
Key changes:
- Adds a new npm script
makemanual
that moves tests from generated to manual directory - Implements tracking of manual tests to skip them during regeneration
- Updates the conversion script to respect manual test exclusions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
package.json | Adds new npm script makemanual for manual test management |
internal/fourslash/_scripts/makeManual.mts | New script that moves tests to manual directory and tracks them |
internal/fourslash/_scripts/convertFourslash.mts | Updates conversion logic to skip manual tests during regeneration |
@@ -20,7 +20,7 @@ func TestCompletionListInClosedFunction05(t *testing.T) { | |||
f.VerifyCompletions(t, "1", &fourslash.CompletionsExpectedList{ | |||
IsIncomplete: false, | |||
ItemDefaults: &fourslash.CompletionsExpectedItemDefaults{ | |||
CommitCharacters: &[]string{}, | |||
CommitCharacters: &defaultCommitCharacters, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a silly question but why can't we generate this one automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no simplified way to compute what the commit characters should be other than the actual implementation. The porting script currently sets empty commit characters if isNewIdentifierLocation
is true, which is an approximation that does not always hold, like in this test. An alternative would be to ignore commit characters for all ported tests, but I didn't want to do that and end up with no coverage of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to just keep a list of these in the generator script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this case, probably, but there are other tests that need other types of manual fixes, and tests that will need to be at least partially manually ported because they have things like for loops and functions.
After tests are generated with
npm run convertfourslash
, if we need to edit them manually, then this script can be run bynpm run manualtest testname
. This creates amanual
folder underfourslash/tests/
and adds the name of the test tomanualTests.txt
.manualTests.txt
will ne skipped and not regenerated when convertfourslash is run again.manual
folder is created,testOutput = cp.execFileSync(go, ["test", "./internal/fourslash/tests/gen"], ["test", "./internal/fourslash/tests/manual"], { encoding: "utf-8" })
can be added toupdateFailing.mts
to run all the tests under./manual
and updatefailingtests.txt
accordingly.