Skip to content
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

Make hover integration test pass #2050

Closed
wants to merge 2 commits into from

Conversation

akshita31
Copy link
Contributor

The hover integration test is failing because of a possible race condition between the omnisharp process and the vscode process to access the file that has been added. Hence, if we restart the server before we ask vscode to open it, omnisharp will have read the file and no problem should occur.

PS: I am not sure whether this is the best approach to solve this, because we will have to restart the server whenever we add a file for testing.

@akshita31 akshita31 changed the title Make hover integration test run Make hover integration test pass Feb 17, 2018
@rchande
Copy link

rchande commented Feb 17, 2018

@akshita31 The solution we discussed was to just always include the "hover" file in the test solution. However, if this works, I'm happy to take it :). I restarted your PR build to see if it's flaky.


await vscode.workspace.openTextDocument(fileUri);

await vscode.commands.executeCommand("vscode.open", fileUri);
Copy link

Choose a reason for hiding this comment

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

Why did you move away from the openTextDocument helper that you switched to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason. I just thought it would maintain consistency with the codeActionRename test. Is openTextDocument a better way ?

@akshita31
Copy link
Contributor Author

@rchande I was trying to add a test for signature help for this PR : #1958 , following the same approach as above, but it doesn't seem to work :(

@akshita31
Copy link
Contributor Author

This approach doesn't work. Closing this in favor of #2058

@akshita31 akshita31 closed this Feb 21, 2018
@akshita31 akshita31 deleted the integrationTest branch February 21, 2018 02:30
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.

2 participants