Skip to content

Fix fourslash tests - #62641

Merged
Andrew Branch (andrewbranch) merged 3 commits into
microsoft:mainfrom
andrewbranch:fix-fourslash-prefs
Oct 20, 2025
Merged

Fix fourslash tests#62641
Andrew Branch (andrewbranch) merged 3 commits into
microsoft:mainfrom
andrewbranch:fix-fourslash-prefs

Conversation

@andrewbranch

@andrewbranch Andrew Branch (andrewbranch) commented Oct 20, 2025

Copy link
Copy Markdown
Member

Fixes incorrect completions preferences passing and trailing commas in package.jsons

Copilot AI review requested due to automatic review settings October 20, 2025 18:17
@github-project-automation github-project-automation Bot moved this to Not started in PR Backlog Oct 20, 2025
@typescript-bot TypeScript Bot (typescript-bot) added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 20, 2025
Copilot AI reviewed Oct 20, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 fixes issues in fourslash tests related to invalid JSON syntax and incorrect completions API usage. The changes ensure that package.json files in test fixtures are valid JSON (removing trailing commas) and that completion preferences are passed correctly to the verify.completions API.

  • Removes trailing commas from package.json files to ensure valid JSON syntax
  • Fixes incorrect preferences passing in verify.completions calls where preferences were mistakenly placed as a separate argument instead of within the options object
  • Adds runtime validation to catch future instances of incorrect preferences passing

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/cases/fourslash/server/importSuggestionsCache_invalidPackageJson.ts Removes trailing comma from jsconfig.json
tests/cases/fourslash/organizeImportsReactJsxDev.ts Removes trailing comma from react package.json
tests/cases/fourslash/organizeImportsReactJsx.ts Removes trailing comma from react package.json
tests/cases/fourslash/completionsWithDeprecatedTag9.ts Fixes preferences passing and adds missing completion entries
tests/cases/fourslash/completionsThisProperties_globalType.ts Fixes preferences passing syntax
tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports48.ts Removes trailing comma from react package.json
tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports47.ts Removes trailing comma from react package.json
tests/cases/compiler/jsxClassAttributeResolution.tsx Removes trailing comma from @types/react package.json
tests/baselines/reference/jsxClassAttributeResolution.js Updates baseline to match fixed package.json
tests/baselines/reference/jsxClassAttributeResolution.errors.txt Updates baseline to match fixed package.json
src/harness/fourslashInterfaceImpl.ts Adds validation to detect incorrect preferences passing pattern

Comment thread src/harness/fourslashInterfaceImpl.ts
Comment on lines 281 to +282
for (const options of optionsArray) {
if (options.preferences && Object.keys(options).length === 1 && optionsArray.length > 1 && optionsArray.indexOf(options) > 0) {

Copilot AI Oct 20, 2025

Copy link

Choose a reason for hiding this comment

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

Using optionsArray.indexOf(options) within a loop has O(n²) complexity. Store the index from the for...of loop instead. Change for (const options of optionsArray) to for (let i = 0; i < optionsArray.length; i++) and use i > 0 in the condition.

Suggested change
for (const options of optionsArray) {
if (options.preferences && Object.keys(options).length === 1 && optionsArray.length > 1 && optionsArray.indexOf(options) > 0) {
for (let i = 0; i < optionsArray.length; i++) {
const options = optionsArray[i];
if (options.preferences && Object.keys(options).length === 1 && optionsArray.length > 1 && i > 0) {

Copilot uses AI. Check for mistakes.

@andrewbranch Andrew Branch (andrewbranch) Oct 20, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not a completely wild review comment but not a concern here

jakebailey
Jake Bailey (jakebailey) reviewed Oct 20, 2025
View reviewed changes
Comment thread tests/cases/fourslash/server/importSuggestionsCache_invalidPackageJson.ts Outdated
@@ -3,7 +3,7 @@
// @Filename: /home/src/workspaces/project/jsconfig.json
////{
//// "compilerOptions": {
//// "module": "commonjs",
//// "module": "commonjs"

@jakebailey Jake Bailey (jakebailey) Oct 20, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a jsconfig so probably okay, but NBD

@andrewbranch Andrew Branch (andrewbranch) Oct 20, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh yeah, I just modified readJson and went to town

@jakebailey Jake Bailey (jakebailey) Oct 20, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does that mean jsconfigs are not using JSONC? Fun

@github-project-automation github-project-automation Bot moved this from Not started to Needs merge in PR Backlog Oct 20, 2025
@andrewbranch

Andrew Branch (andrewbranch) commented Oct 20, 2025

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) cherry-pick to tsgo-port

@typescript-bot

TypeScript Bot (typescript-bot) commented Oct 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
cherry-pick to tsgo-port ✅ Started ✅ Results

Merged
@typescript-bot

TypeScript Bot (typescript-bot) commented Oct 20, 2025

Copy link
Copy Markdown
Contributor

Hey, Andrew Branch (@andrewbranch)! I've created #62642 for you.

@andrewbranch

Andrew Branch (andrewbranch) commented Oct 20, 2025

Copy link
Copy Markdown
Member Author

Lol, one of them is named "importSuggestionsCache_invalidPackageJson" so I better revert that one too

@andrewbranch

Andrew Branch (andrewbranch) commented Oct 20, 2025

Copy link
Copy Markdown
Member Author

Oh, it’s the same one I changed the jsconfig on

@andrewbranch

Andrew Branch (andrewbranch) commented Oct 20, 2025

Copy link
Copy Markdown
Member Author

These timeouts are getting annoying...

@andrewbranch
Andrew Branch (andrewbranch) merged commit 84f4856 into microsoft:main Oct 20, 2025
56 of 60 checks passed
@github-project-automation github-project-automation Bot moved this from Needs merge to Done in PR Backlog Oct 20, 2025
Andrew Branch (andrewbranch) added a commit that referenced this pull request Oct 20, 2025
@typescript-bot @andrewbranch
🤖 Pick PR #62641 (Fix fourslash tests) into tsgo-port (#62642)
050a6f8 
Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com>
Co-authored-by: Andrew Branch <andrew@wheream.io>
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Apr 20, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jakebailey Jake Bailey (jakebailey) jakebailey approved these changes

Assignees

@andrewbranch Andrew Branch (andrewbranch)

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

PR Backlog
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@andrewbranch @typescript-bot @jakebailey