Raise the default warning level in tests #47077
Conversation
jcouv
added
Area-Compilers
Community
src/Compilers/Core/CodeAnalysisTest/Diagnostics/CompilationWithAnalyzersTests.cs
Outdated
Show resolved
Hide resolved
| @@ -37,6 +37,8 @@ public Exception LoadAnalyzer(string analyzerPath) | |||
|
|
|||
| public class AnalyzerFileReferenceAppDomainTests : TestBase | |||
| { | |||
| private const int MaxWarningLevel = 9999; | |||
There was a problem hiding this comment.
Because this project doesn't have a reference to TestOptions, I created this const. But I don't feel this approach is good. This const is defined in a few other tests in this project.
There was a problem hiding this comment.
Perhaps it would be reasonable to define it next to
There was a problem hiding this comment.
Why isn't max warning level just int.MaxValue ?
There was a problem hiding this comment.
It should work, but it was documented here as 9999 so I matched that.
|
@jaredpar @RikkiGibson I've addressed your feedback and also updated more tests (there are still a few other tests that needs to be updated too). |
src/Compilers/CSharp/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.cs
Show resolved
Hide resolved
src/Compilers/Core/CodeAnalysisTest/Diagnostics/CompilationWithAnalyzersTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit/PDB/CSharpDeterministicBuildCompilationTests.cs
Outdated
Show resolved
Hide resolved
|
Pinging @RikkiGibson |
|
I think we should include the new warning in |
|
@RikkiGibson I included it. Can you re-run the failed jobs? the current failures seem unrelated to the changes. If this is ready to go, you may want consider a squash/merge. The number of commits here is unnecessarily large |
|
@RikkiGibson Build is green now. |
|
@dotnet/roslyn-compiler for another review. It's a test only change, but sweeping enough that it feels like getting another pair of eyes on it would be good. Suggest starting with the following files:
Then skimming the rest of it. |
|
Looking |
|
Note: commit-by-commit review is not recommended at all, the commits here are messy. I'd also recommend a squash/merge to not pollute the master history. |
| /// <param name="optimizationLevel">The optimization level of the created compilation options.</param> | ||
| /// <param name="allowUnsafe">A boolean specifying whether to allow unsafe code. Defaults to false.</param> | ||
| /// <returns>A CSharpCompilationOptions with the specified <paramref name="outputKind"/>, <paramref name="optimizationLevel"/>, and <paramref name="allowUnsafe"/>.</returns> | ||
| internal static CSharpCompilationOptions CreateTestOptions(OutputKind outputKind, OptimizationLevel optimizationLevel, bool allowUnsafe = false) |
There was a problem hiding this comment.
internal [](start = 8, length = 8)
private? Never mind. Somehow did a bad search ;-) #Closed
|
@jcouv That was already done. The current CI run was succeeded 2 hours ago in 1h 44m 11s |
ghost
added this to the
|
Thanks for the contribution @Youssef1313! |
Fixes #46976
@RikkiGibson Could you have a look if what I'm doing until now is the correct thing?