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 to configure diagnostic severity via editorconfig #46639

Merged
merged 1 commit into from
Aug 10, 2020

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented Aug 7, 2020

No description provided.

@mavasani mavasani requested a review from a team as a code owner August 7, 2020 19:40
jasonmalinowski added a commit to jasonmalinowski/roslyn that referenced this pull request Aug 7, 2020
For reasons that aren't entirely clear, when we create a preview of
a code fix, we remove and re-add documents that are being changed when
we produce the PreviewWorkspace. This process had an bug which meant
that the file path of an .editorconfig got dropped, so the resultant
.editorconfig document path was null. This later caused a crash if
the diagnostic engine, when processing the PreviewWorkspace's
new solution, tried asking for a Compilation, since the null path would
get passed to AnalyzerConfig.Parse(), and it would throw.

This was exposed by dotnet#45076; until then if you had a preview of just
an .editorconfig file, nothing would actually be analyzed and so
nothing asked for the Compilation. The crash is "fixed" by dotnet#44331 in
that asking for a Compilation no longer results in a call to
AnalyzerConfig.Parse(); you have to ask for a Compilation and then do
something with a semantic model. By luck, that doesn't happen in the
common repro, but more obscure things absolutely could still fail.

This is being tested by the integration test being added in dotnet#46639. We
don't really have a good unit tests here as far as I can tell, but even
then a unit test really doesn't validate any of the end-to-end here.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1162464
@mavasani
Copy link
Contributor Author

Ping @jasonmalinowski @dotnet/roslyn-ide

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

:shipit: Even if we can make this validate the preview, no reason to hold this up before we get that in.

"Warning",
"Error",
};
VisualStudio.Editor.Verify.CodeActions(expectedItems, applyFix: "Error", ensureExpectedItemsAreOrdered: true);
Copy link
Member

Choose a reason for hiding this comment

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

Is there some way to have this validate the preview?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to thread in a delay between the action being computed and applied, and the integration test does wait after bringing up the light bulb, but the integration test does not expand the nested actions and hence do not bring up the preview. We either need to figure out some other way to test preview OR write a dummy test analyzer that registers a top level code action to update the .editorconfig, though I am not sure if our integration tests can run with a custom defined analyzer that is not in the box.

In short, I could not find an easy way to validate the preview.

@mavasani
Copy link
Contributor Author

I'll merge this test as it seems useful by itself, even if it doesn't validate the preview currently - it will still validate applying a code fix from IDE that updates editorconfig and validates the change in reported diagnostic severity.

@mavasani mavasani merged commit 8331c78 into dotnet:master Aug 10, 2020
@ghost ghost added this to the Next milestone Aug 10, 2020
@mavasani mavasani deleted the ConfigureSeverityIntegrationTest branch August 10, 2020 19:27
@RikkiGibson RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020
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.

3 participants