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

Add integration test for .runsettings in unit tests #6336

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

dibarbet
Copy link
Member

Followup for #6265 (comment)

@dibarbet dibarbet requested a review from a team as a code owner September 11, 2023 20:15

async function setConfigurationAndWaitForObserver<T>(configuration: string, value: T): Promise<void> {
const changed = new Promise<void>((resolve, _) => {
vscode.workspace.onDidChangeConfiguration((e) => {
Copy link
Member Author

@dibarbet dibarbet Sep 11, 2023

Choose a reason for hiding this comment

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

incredibly annoyingly, optionProvider.GetLatestOptions() does not actually read the latest options. It instead uses whatever the observable last triggered with. This is extremely racy, so I workaround this by explicitly waiting for the event to fire.

It isn't simple to modify GetLatestOptions() because we don't want to be reading the entire option set every single time. Instead I think I need to refactor options to instead return functions to get the latest value, instead of the entire set of options.

Copy link
Member Author

Choose a reason for hiding this comment

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

filed #6337 for this

@dibarbet dibarbet force-pushed the runsettings_integration_test branch from b91d7e4 to 5d3a063 Compare September 11, 2023 20:29
@dibarbet dibarbet merged commit 17ef56a into dotnet:main Sep 11, 2023
8 checks passed
@dibarbet dibarbet deleted the runsettings_integration_test branch September 11, 2023 21:17
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