-
Notifications
You must be signed in to change notification settings - Fork 255
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
Special test scenario with new unit test framework fails #422
Comments
This sounds like a case where it would be necessary to explicitly provide the expected diagnostics for the fixed state. |
Technically this problem will only arise when dealing with compiler diagnostics anyway, right? In that case it is just just more verbose to deal with advanced scenarios |
Not necessarily. It is also known to occur in a few other cases:
|
Hm. Still getting unexpected behavior in my test method here: neither one of those calls in my refactoring branch (refactoring-remove-old-testframework) work. The diagnostics related to my Analyzer seem to be excluded. Can you please spot my test for any errors? For the simple syntax i get 0/4 diagnostics. |
The clue is here:
The test is failing because the test framework added the following comment to the top of the input file, but the diagnostic failed to go away:
If your analyzer intends to report diagnostics in generated code, it needs to use the full form for testing and disable the test for generated code. await new VerifyCS.Test {
TestBehaviors = TestBehaviors.SkipGeneratedCodeCheck,
// ...
}.RunAsync(); If the analyzer does not need to report diagnostics in generated code, then it should be configured in the context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); |
Thanks! that worked. What do you think about perhaps adding such remarks to the output of a test? In the past when working with dotnet tools i recently noticed how sometimes they include indicators of what settings to change if you run into edge cases which solve your problem? |
That seems like a good idea. It could either print the message directly, or it could link to a help page in the documentation for the test library. |
Combining both (while keeping the notice short) would be useful. Personally i was unaware of this configuration and anyone who is interested in learning about would have a place to read up on it. |
@sharwell what are your thoughts on introducing a settings property to alter the values of the test prior to running it - the way it is right now i have to use a lot of verbose versions instead of even being able to use the simple call. e.g. turning
into
That way inheriting from the test would not be necessary anymore and changing the level of compiler diagnostics would be simple + setting the behavior would also involve less code. |
For example this: var expected = new[]
{
// Test0.cs(24,21): info ACA0001: Forward execution of "Method1" to member "_disposables"
Verifier.Diagnostic().WithSpan(24, 21, 24, 28).WithArguments("Method1", "_disposables"),
// Test0.cs(33,21): info ACA0001: Forward execution of "Method2" to member "_disposables"
Verifier.Diagnostic().WithSpan(33, 21, 33, 28).WithArguments("Method2", "_disposables"),
// Test0.cs(38,21): info ACA0001: Forward execution of "Method2" to member "_disposables"
Verifier.Diagnostic().WithSpan(38, 21, 38, 28).WithArguments("Method2", "_disposables"),
// Test0.cs(43,21): info ACA0001: Forward execution of "Method3" to member "_disposables"
Verifier.Diagnostic().WithSpan(43, 21, 43, 28).WithArguments("Method3", "_disposables")
};
await Verifier.VerifyCodeFixAsync(test, expected, fixtest); right now gives me this: This could be fixed by simply turning the code to var expected = new[]
{
// Test0.cs(24,21): info ACA0001: Forward execution of "Method1" to member "_disposables"
Verifier.Diagnostic().WithSpan(24, 21, 24, 28).WithArguments("Method1", "_disposables"),
// Test0.cs(33,21): info ACA0001: Forward execution of "Method2" to member "_disposables"
Verifier.Diagnostic().WithSpan(33, 21, 33, 28).WithArguments("Method2", "_disposables"),
// Test0.cs(38,21): info ACA0001: Forward execution of "Method2" to member "_disposables"
Verifier.Diagnostic().WithSpan(38, 21, 38, 28).WithArguments("Method2", "_disposables"),
// Test0.cs(43,21): info ACA0001: Forward execution of "Method3" to member "_disposables"
Verifier.Diagnostic().WithSpan(43, 21, 43, 28).WithArguments("Method3", "_disposables")
};
await Verifier.VerifyCodeFixAsync(test, expected, fixtest, config => config.TestBehaviors = TestBehaviors.SkipGeneratedCodeCheck);
or ofc:
await Verifier.VerifyCodeFixAsync(test, expected, fixtest, SomeConfiguration); instead of await new CodeFixTest<Analyzer, FixByForwardingToCollectionChildren>()
{
TestBehaviors = TestBehaviors.SkipGeneratedCodeCheck,
CompilerDiagnostics = CompilerDiagnostics.Errors,
TestState =
{
Sources = {test},
ExpectedDiagnostics =
{
// Test0.cs(24,21): info ACA0001: Forward execution of "Method1" to member "_disposables"
Verifier.Diagnostic().WithSpan(24, 21, 24, 28).WithArguments("Method1", "_disposables"),
// Test0.cs(33,21): info ACA0001: Forward execution of "Method2" to member "_disposables"
Verifier.Diagnostic().WithSpan(33, 21, 33, 28).WithArguments("Method2", "_disposables"),
// Test0.cs(38,21): info ACA0001: Forward execution of "Method2" to member "_disposables"
Verifier.Diagnostic().WithSpan(38, 21, 38, 28).WithArguments("Method2", "_disposables"),
// Test0.cs(43,21): info ACA0001: Forward execution of "Method3" to member "_disposables"
Verifier.Diagnostic().WithSpan(43, 21, 43, 28).WithArguments("Method3", "_disposables")
},
},
FixedState =
{
ExpectedDiagnostics =
{
},
Sources = {fixtest}
},
}.RunAsync(); I know the Configure call, can allow me to fix my test by altering the mode, but from looking at msdocs articles regarding the enum values i can't quite tell whether it is actually okay to change it to None. |
I'm not a big fan of this right now, for two reasons:
I don't see this as a particular advantage. I find the inherited types quite convenient for tailoring the test framework to the majority characteristics of an analyzer library. |
There is no need to set properties to their default values. For your example:
|
option 2 seems like a viable alternative compared to a configuration delegate, yes. Yes, it is good that this is possible, but it feels awkward to use in simple use cases because of the required verbosity. I agree that this option should always exist, because it allows highly granular control over how the test will be evaluated, but having the opportunity of slim test cases with low boilerplate should also be an option. |
The problem here is each analyzer library tends to have different "minimum functionality requirements". Accounting for each variation of "minimum" results in a library that isn't simple or minimum for anyone. The main advantage of starting with an inherited type in your project is you can define the minimum API as it applies to the patterns/needs of the analyzer project in question, without concerning yourself with the details of any other analyzer. |
Makes sense. In that case perhaps there just have to be a couple more steps in the verifier which need to be extendable to get the desired testing behavior within the scope of an analyzer repo, where the written analyzers are not concerned with codegeneration or the consumer wants to alter the infered expected result generation, you could just do that then. |
This was fixed by #569. If the analyzer calls |
I've got a scenario where my test case has an error and a warning - i provide a codefix or the warning. After the warning is fixed, the error is fixed automatically too.
If i expect both diagnostics get this:
![grafik](https://user-images.githubusercontent.com/5545184/72643849-e129fd80-396f-11ea-901c-ce1c1f2cdbc9.png)
If i expect just one diagnostic i get this:
![grafik](https://user-images.githubusercontent.com/5545184/72643926-0d457e80-3970-11ea-9c2f-8ffe51ed4ffd.png)
The old Inherited CodeFixTest version makes the test pass just fine.
this is my test method:
This is my CodeFix:
@sharwell Willing to contribute again if this is an option. Unless you want to take care of it yourself of course :)
The text was updated successfully, but these errors were encountered: