Skip to content

Conversation

@jjonescz
Copy link
Member

@jjonescz jjonescz commented May 2, 2024

Part of #10247.

I will look into verifying C# diagnostics in other tests as well, but I wanted to get some early feedback before I continue.

I have changed the tests to reference MVC shims, added more razor compiler passes, and more things to avoid many noisy diagnostics. But note that many errors still remain in many of them because they were simply never written to successfully compile. But I think that's fine for baselines - we should be just cautious when in future some PRs add to the diagnostics.

Also if you prefer to not change the tests at all and have more noisy diagnostics as baselines instead, I can do that too.

@jjonescz jjonescz added the area-compiler Umbrella for all compiler issues label May 2, 2024
@jjonescz jjonescz force-pushed the verify-everywhere branch from 49c353b to 246cfba Compare May 2, 2024 14:12
@jjonescz jjonescz marked this pull request as ready for review May 2, 2024 14:45
@jjonescz jjonescz requested review from a team as code owners May 2, 2024 14:45
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

I've gone through a few of the test files, but I'll put off going through them all until my other questions are sorted to avoid any churn that results. Overall, this is looking good, and I'm super happy to see it. Thanks for the effort Jan!

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

:shipit:

@jjonescz
Copy link
Member Author

jjonescz commented May 6, 2024

@chsienki @dotnet/razor-compiler for a second review, thanks

Copy link
Member

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

LGTM. Consider moving the class properties onto the IntegrationTestFact if we think there will be more of these or keep as-is if these are expected to be very limited.

{
// C# 8 features are not available in .NET Framework without polyfills
// so the C# diagnostics would be different between .NET Framework and .NET Core.
SkipVerifyingCSharpDiagnostics = ExecutionConditionUtil.IsDesktop;
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider making these optional params to the IntegrationTestFact? So we could have e.g. [IntegrationTestFact(SkipVerifyingCSharpDiagnostics = true)]

Copy link
Member Author

@jjonescz jjonescz May 10, 2024

Choose a reason for hiding this comment

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

Hm, haven't considered that, sounds interesting. But it looks like it would be much more difficult to propagate these options down to the IntegrationTestBase methods where I need them. If I understand correctly, I would need to add new properties to IntegrationTestFact, then add new parameters to constructors of each test class, then pass the properties as constructor arguments in IntegrationTestCase. Maybe there's a simpler way? But still, for end-user the effect would be just moving something like SkipVerifyingCSharpDiagnostics = ExecutionConditionUtil.IsDesktop; from the test body into the attribute which doesn't seem worth the effort here. So I think I will leave as is. But happy to change this in a follow up if I misunderstood :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-compiler Umbrella for all compiler issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants