-
Notifications
You must be signed in to change notification settings - Fork 231
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
Metrics analyzer for Razor: Lines of code are outside the range of the file #9299
Conversation
57b9c6b
to
a37fa52
Compare
@@ -1 +1 @@ | |||
Roslyn version: 4.8.0.0Language version: CSharp12!Concurrent execution: enabled��File 'Microsoft.NET.Sdk.Razor.SourceGenerators\Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator\OverlapSymbolReferences_razor.g.cs' was recognized as razor generated��File 'Microsoft.NET.Sdk.Razor.SourceGenerators\Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator\Cases_razor.g.cs' was recognized as razor generatedvrFile 'C:\Users\martin.strecker\Desktop\WebAss\obj\Debug\net8.0\WebAss.AssemblyInfo.cs' was recognized as generated��File 'C:\Users\martin.strecker\Desktop\WebAss\obj\Debug\net8.0\.NETCoreApp,Version=v8.0.AssemblyAttributes.cs' was recognized as generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we keep the old tests as well? (for regression testing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reflect the behavior of both versions of Roslyn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
16c6016
to
1a47df3
Compare
6fb9664
to
f2c30bb
Compare
- and update the pb files with data produced by dotnet 8.0.5
f2c30bb
to
948fcd3
Compare
...rary/src/test/java/org/sonarsource/dotnet/shared/plugins/protobuf/RazorImporterTestBase.java
Show resolved
Hide resolved
...y/src/test/java/org/sonarsource/dotnet/shared/plugins/protobuf/RazorMetricsImporterTest.java
Show resolved
Hide resolved
Quality Gate passed for 'Sonar .NET Java Plugin'Issues Measures |
Quality Gate passed for 'SonarAnalyzer for .NET'Issues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey dude, this looks very nice and well-tested.
I left a couple of questions, mostly for curious/educational reasons.
I don't assume many changes will be needed, after my questions it will probably be approved. :)
...ed-library/src/main/java/org/sonarsource/dotnet/shared/plugins/protobuf/MetricsImporter.java
Show resolved
Hide resolved
...y/src/test/java/org/sonarsource/dotnet/shared/plugins/protobuf/RazorMetricsImporterTest.java
Show resolved
Hide resolved
.collect(Collectors.groupingBy(Function.identity(), Collectors.counting())); | ||
|
||
for (int groupKey : groups.keySet()){ | ||
verify(context, times(groups.get(groupKey).intValue())).setIntValue(key, groupKey, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a bit hard to understand what you are doing here.
Why do you group the lines and then do times
?
From debugging they are always the unique.
How could it be that a group has more than one occurrence of each line?
Do you want to make sure exactly this? That every line appears ONLY once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When multiple files with test cases are added to the context, we can have lines reported multiple times. This was the behavior before my changes when the test base class added files to the context.
Strictly speaking, this is now used only to check that each element is added only once, and the query can be simplified. I wanted to have this method able to check duplicates, so we can easily add test cases in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the explanations!
Fixes #9288
For performance reasons, I want to avoid counting the file lines in the utility analyzers. To reduce the IO operations during analysis. An additional reason is that any information we get about invalid data will be lost during analysis (we don't have logs).
The idea is to do a check on the plugin side to see if the data we send to the server is valid. If it's not, we will log a debug message with the file name and the line number.