Skip to content

WIP Porting more completion tests and adding text change methods #8311

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

WIP Porting more completion tests and adding text change methods #8311

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97fbb78
Porting more completion tests and adding text change methods that ing…
alexgav May 20, 2025
c294504
stabilizing tests
alexgav May 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 129 additions & 1 deletion test/razor/razorIntegrationTests/completion.integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ integrationHelpers.describeIfDevKit(`Razor Hover ${testAssetWorkspace.descriptio
vscode.CompletionItemKind.TypeParameter,
'text'
);

await clearLine(4);
});

test('Div Tag', async () => {
Expand All @@ -65,8 +67,128 @@ integrationHelpers.describeIfDevKit(`Razor Hover ${testAssetWorkspace.descriptio
'div',
expectedDocumentation
);

await clearLine(6);
}, 30000);

test('Disabled Attribute', async () => {
if (!integrationHelpers.isRazorWorkspace(vscode.workspace)) {
return;
}

await insertText(new vscode.Position(6, 0), '<button dis');

await waitForExpectedCompletionItemAsync(
new vscode.Position(6, 11),
undefined,
undefined,
'disabled',
vscode.CompletionItemKind.Value,
'disabled'
);

await clearLine(6);
}, 30000);

test('Dir Attribute', async () => {
if (!integrationHelpers.isRazorWorkspace(vscode.workspace)) {
return;
}

await insertText(new vscode.Position(6, 0), '<div dir=');

await waitForExpectedCompletionItemAsync(
new vscode.Position(6, 9),
undefined,
undefined,
'auto',
vscode.CompletionItemKind.Unit,
'"auto"'
);

await clearLine(6);
}, 30000);

test('C# String Type', async () => {
if (!integrationHelpers.isRazorWorkspace(vscode.workspace)) {
return;
}

await insertText(new vscode.Position(8, 0), 'str');

await waitForExpectedCompletionItemAsync(
new vscode.Position(8, 3),
undefined,
undefined,
'string',
vscode.CompletionItemKind.Keyword,
'string'
);

await clearLine(8);
}, 30000);

test('C# Override', async () => {
if (!integrationHelpers.isRazorWorkspace(vscode.workspace)) {
return;
}

await insertText(new vscode.Position(8, 0), 'override OnA');

await waitForExpectedCompletionItemAsync(
new vscode.Position(8, 12),
undefined,
undefined,
'OnAfterRender(bool firstRender)',
vscode.CompletionItemKind.Method,
// This is not correct - filed https://github.com/dotnet/vscode-csharp/issues/8310
'OnA'
);

await clearLine(8);
}, 30000);

async function insertText(position: vscode.Position, text: string): Promise<void> {
const activeEditor = vscode.window.activeTextEditor;
if (!activeEditor) {
throw new Error('No active editor');
}

try {
await activeEditor.edit((builder) => {
builder.insert(position, text);
});
} catch (error) {
if (error instanceof vscode.CancellationError) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit suspicious to be catching the cancellation error here specifically. It looks like the test itself is attempting to insert the text, but the insert gets cancelled. Wouldn't that fail the test because the text did not successfully get applied?

I wouldn't expect the actual insertion to get cancelled in general, unless someone else is changes the file underneath. I would generally expect other requests (e.g. a classification request) to get cancelled if you change the file - but I don't think that would cause the edit application itself to throw a cancellation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cancellations and retries happen in razorDocumentSynchronizer during the text insertion. Text actually gets inserted and document gets synchronized eventually (thus re-tries while wiating for expected completion item).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though looking at the failures from the test run, I am catching them in the wrong place. At least some of the time synchronization appears to happen after the insertion/deletion (e.g. outside of insertText). I guess I could wrap test method itself including and after the modification operations.

Copy link
Member

Choose a reason for hiding this comment

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

At least some of the time synchronization appears to happen after the insertion/deletion (e.g. outside of insertText). I guess I could wrap test method itself including and after the modification operations.

So if the synchronizer is changing the document underneath you may have to catch the exception here, think I get that part now (since the edit may fail).

If you're seeing them thrown outside of the insert text part, you probably want to catch them in the actual synchronization code path then? Unless some caller of it needs to observe the cancellation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't throw explicitly, at least not that I can easily see - I'll debug through that some more

vscode-csharp/src/razor/src/document/razorDocumentSynchronizer.ts

Line 143 in 3c12e27

context.cancel(vscode.l10n.t('Token cancellation requested: {0}', reason));

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the rejection is happening outside of the await? The rejection happens for either onCancellationRequested or setTimeout. I'm not sure if those events could be triggered separately from the await, and therefore cause the promise rejection to not be handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debugging it still, unfortunately the issue is pretty intermittent on my machine (though CI machines have no problem hitting it :) ).

Copy link
Member

Choose a reason for hiding this comment

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

fwiw the code in there seems quite complex and a bit suspect in general. I wonder if it would be worth rewriting it instead of spending a bunch of time debugging. I'm no TS expert, but the multiple events rejecting the same promise, possibly entirely outside of the await context seems very prone to random issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I am neither TS expert nor razorDocumentSynchronizer expert, so I'm not up to the task of re-writing it. I debugged some more and tried a few things.

Cancellation seems to occur in request processing in request queue when previous requests of the same type is already running, which makes sense I guess. As we are inserting the text, we may hit previous request still running.

image

The unhandled error that jest sees is simply a string, not vscode.CancellationError. Also copilot did say it's a good practice to catch errors from rejection, so I added some catch clauses to the promises that seem to be the source of the errors. It seems to work on my machine, unless it's just flaky and not hitting the issue locally. I pushed the current fix and will see if the tests are passing on the CI now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After my last commit hover and rename tests are failing (though not on my box of course). I'll convert this PR to draft and mark with WIP while I figure out what's going on.

return;
}

throw error;
}
}

async function clearLine(lineNumber: number): Promise<void> {
const editor = vscode.window.activeTextEditor;
if (!editor) {
return;
}

const lineObj = editor.document.lineAt(lineNumber);
const textRange = new vscode.Range(lineObj.range.start, lineObj.range.end);

try {
await editor.edit((editBuilder) => {
editBuilder.delete(textRange);
});
} catch (error) {
if (error instanceof vscode.CancellationError) {
return;
}

throw error;
}
}

async function getCompletionsAsync(
position: vscode.Position,
triggerCharacter: string | undefined,
Expand Down Expand Up @@ -117,7 +239,13 @@ integrationHelpers.describeIfDevKit(`Razor Hover ${testAssetWorkspace.descriptio
}
expect(completionItem).toBeDefined();
expect(completionItem!.kind).toEqual(expectedKind);
expect(completionItem!.insertText).toBe(expectedInsertText);

const insertText =
completionItem.insertText instanceof vscode.SnippetString
? completionItem.insertText.value
: completionItem.insertText;
expect(insertText).toBe(expectedInsertText);

if (expectedDocumentation) {
const documentation = completionItem!.documentation as vscode.MarkdownString;
expect(documentation.value).toBe(expectedDocumentation);
Expand Down
Loading