Add support for Deterministic Source Paths (VS Coverage format) #3817

andrei-epure-sonarsource · 2020-12-07T20:06:04Z

andrei-epure-sonarsource · 2020-12-07T20:11:12Z

@pavel-mikula-sonarsource , if you want, you can have an initial look. Currently, this implementation is not working (at least in the ITs - which is an artificial use case, as the coverage file is checked in and the code runs under a temporary folder - see the details in 6969a9a commit message).

Left things:

have a working integration test
do manual testing on Windows and Linux
add an integration test with _, _1 and _2 placeholders inside the deterministic source paths (which won't work, as this PR is an initial baby step)
add a setting to enable this feature - there are a few things that could go wrong, like CI systems where _ may be the starting folder (hard to predict) - I would rather have this feature require an explicit opt-in , get feedback , and avoid live site incidents

andrei-epure-sonarsource · 2020-12-10T16:00:15Z

@pavel-mikula-sonarsource I should still test this manually on Linux , and maybe with a more complex project than the one in the integration tests

Also, please share your thoughts if these are necessary

add an integration test with _, _1 and _2 placeholders inside the deterministic source paths (which won't work, as this PR is an initial baby step)
add a setting to enable this feature - there are a few things that could go wrong, like CI systems where _ may be the starting folder (hard to predict) - I would rather have this feature require an explicit opt-in , get feedback , and avoid live site incidents

...tnet-shared-library/src/test/java/org/sonar/plugins/dotnet/tests/ScannerFileServiceTest.java

...ry/src/test/java/org/sonar/plugins/dotnet/tests/VisualStudioCoverageXmlReportParserTest.java

...r-dotnet-shared-library/src/main/java/org/sonar/plugins/dotnet/tests/ScannerFileService.java

...tnet-shared-library/src/test/java/org/sonar/plugins/dotnet/tests/ScannerFileServiceTest.java

andrei-epure-sonarsource · 2020-12-11T11:52:00Z

I opened #3832 to improve coverage statistics in the logging.

pavel-mikula-sonarsource

I left some questions and improvement suggestions.

add an integration test with _, _1 and _2 placeholders

I'd add just UTs. In one of the comments, I suggested implementing those, as it's easy.

add a setting to enable this feature

Can you demonstrate a case in UTs with what could go wrong? Having CI with _ root will just do the relative-path magic and should end in the same place as the absolute path. Shouldn't it?

I'm in for keeping it as out of the box feature. Coverage has already some pain to configure.

its/projects/CoverageWithDeterministicSourcePaths/VisualStudio.coveragexml

...-dotnet-shared-library/src/main/java/org/sonar/plugins/dotnet/tests/NCover3ReportParser.java

sonar-dotnet-shared-library/src/main/java/org/sonar/plugins/dotnet/tests/FileService.java

...ry/src/test/java/org/sonar/plugins/dotnet/tests/VisualStudioCoverageXmlReportParserTest.java

...ibrary/src/main/java/org/sonar/plugins/dotnet/tests/VisualStudioCoverageXmlReportParser.java

...tnet-shared-library/src/test/java/org/sonar/plugins/dotnet/tests/ScannerFileServiceTest.java

...ry/src/test/java/org/sonar/plugins/dotnet/tests/VisualStudioCoverageXmlReportParserTest.java

andrei-epure-sonarsource · 2020-12-14T12:29:28Z

@pavel-mikula-sonarsource the new commits , addressing your comments, start with "rename method and fields; modify UT" 3add2dd

pavel-mikula-sonarsource

GitHub won't let me add note to existing comment. Please take a look at the note that I will write in a minute :)

...r-dotnet-shared-library/src/main/java/org/sonar/plugins/dotnet/tests/ScannerFileService.java

pavel-mikula-sonarsource

Rewritten comment

...r-dotnet-shared-library/src/main/java/org/sonar/plugins/dotnet/tests/ScannerFileService.java

...tnet-shared-library/src/test/java/org/sonar/plugins/dotnet/tests/ScannerFileServiceTest.java

where the folder is '/D:/a/1/s/sonar-dotnet-shared-library/mod/root/some/path/file.cs'

...r-dotnet-shared-library/src/main/java/org/sonar/plugins/dotnet/tests/ScannerFileService.java

sonarqubecloud · 2020-12-14T16:02:01Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

sonarqubecloud · 2020-12-14T16:17:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

pavel-mikula-sonarsource

One last thing I didn't spot previously.

pavel-mikula-sonarsource · 2020-12-15T10:44:12Z

...r-dotnet-shared-library/src/main/java/org/sonar/plugins/dotnet/tests/ScannerFileService.java

+      Iterable<InputFile> files = fileSystem.inputFiles(fileSystem.predicates().hasLanguage(languageKey));
+      int count = 0;
+      InputFile foundFile = null;
+      for (InputFile file : files) {
+        String path = file.uri().getPath();
+        if (path.endsWith(relativePath)) {
+          count++;
+          foundFile = file;
+        }
+      }


This new layout is great, as it's easy to follow. Now it's way more visible what's going.

Open question: The filtering in the for is actually a manual way of implementing lambda. predicate actually in Java terms. So can we replace this section with something predicate based?

There exists predicates().hasRelativePath(). Does that one solve our issue (I'm not sure if it does the same)? If not, isn't it simpler to implement a predicate? Even in-lined one with the new FilePredicate(){accept(...)} java syntax for inline abstract class implementation. As it should be 3 LOC.

We would not need the count variable.
We would return to the structure I asked you to remove previously :D

FilePredicates fp = fileSystem.predicates(); Iterable<InputFile> files = fileSystem.inputFiles( fp.and( fp.hasRelativePath(path), fp.hasLanguage(languageKey) ));

as previously mentioned during a standup on Friday, this doesn't work (the API doesn't find the file). This is why I ended up doing this logic, because I wanted to have something working.
I need to talk with someone from the scanner team for this - and I should indeed open a ticket for it.

L.E.: (it's unfortunate I didn't capture the notes for this problem in the PR; I need to create a reproducer project and report it to the scanner guild)

I read your suggestion better now, getting it back in progress

Copy from Slack, as I just found out:
The relative path predicate does this:

return path.equals(f.relativePath());

so it will not help us 😞
As seen here: https://github.com/SonarSource/sonar-enterprise/blob/021961b86da874e4b733a566ce252[…]nar/api/batch/fs/internal/predicates/RelativePathPredicate.java
PathPatternPredicate could do the trick, but it doesn't worth the trouble with crafting the dynamic regex
So if you see easy way how to implement own predicate, let's do it. Otherwise, let's merge it as is.

PathPatternPredicate = predicates().matchesPathPattern()

pavel-mikula-sonarsource

LGTM. As discussed, let's refactor the predicate in another PR.

andrei-epure-sonarsource changed the title ~~Andrei/deterministic source paths~~ Add support for Deterministic Source Paths Dec 7, 2020

andrei-epure-sonarsource force-pushed the andrei/deterministic-source-paths branch 2 times, most recently from 77f6fee to f448806 Compare December 10, 2020 14:57

andrei-epure-sonarsource marked this pull request as ready for review December 10, 2020 15:59

andrei-epure-sonarsource requested a review from pavel-mikula-sonarsource December 10, 2020 15:59