Improves Visual studio/MSBUILD log file analysis

[rudolfgrauberger]

Uses regular expression to get the project file indenpendently of the language of the log file. I've tested this with the existing englisch log file and with a new german log file.

This is important, among other things, the following points:

• Special folder structure
• Simplifies configuration

1. Special folder structure

The detecting of correct project file is nessesary if you have following project folder structure

.
├── Build                                # Project files (.vxcproj)
│   ├── project.vcxproj
│   ├── buildlog.log                     # MSBUILD log file
├── Interface                            # Public Header files
├── Source                               # Source files (and internal header files)
├── Test                                 # Tests
└── sonar-project.properties

The relative include paths are then correctly converted relative to the project file.

2. Simplifies configuration

It is very helpful because all INCLUDE and DEFINES are taken from the project file and do not have to be defined manually.

---

This change is 

[guwirth]

Hi @rudolfgrauberger, tests on Travis (Linux) are failing:

[ERROR] Errors: 
[ERROR]   CxxVCppBuildLogParserTest.shouldExtractProjectFileFromEnglishVCpp2013Log:73 NullPointer
[ERROR]   CxxVCppBuildLogParserTest.shouldExtractProjectFileFromGermanVCpp2017Log:105 NullPointer

[rudolfgrauberger]

@guwirth Oh, okay, thanks for the info. I will correct it in the next few days.

[rudolfgrauberger]

@guwirth Ready :-)

[ivangalkin]

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @rudolfgrauberger)

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 75 at r2 (raw file):

softly.assertThat(includes.contains("...")).isTrue();

here and below should be

softly.assertThat(includes).contains("...");

Assertion written in this way will make the error message much more informative

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 90 at r2 (raw file):

We can test this at the moment only if we check how the relative include paths translated to absolute paths

not sure, that I understand the comment. Could you please explain?

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 125 at r2 (raw file):

 }

EOF looks odd. File encoding is not UTF-8?

---

cxx-squid/src/test/resources/logfile/vc++2017-de.txt, line 1 at r2 (raw file):

Der Buildvorgang wurde am 10.01.2019 15:41:59 gestartet.

I am not familiar with MSVC and not sure, if I understood the idea of improvement.

• Is this large file really necessary?
• Does it provides some exemplary or corner cases?
• Does the test above validates some newly implemented feature? If yes, this is not obvious
• Could we use sonar-cxx/cxx-sensors/src/test/resources/org/sonar/cxx/sensors/reports-project/compiler-reports/VC-report.vclog instead? could we extend the existing log instead of adding a new one?

I agree, that the test has to validate some realistic scenario. But how can anybody review the correctness of your test if its input contains > 1500 lines. I would minimize the input and emphasize the subject of the test.

[ivangalkin]

@rudolfgrauberger Thank you for the providing of this PR. I have no strong opinion about the idea of your change. My review addresses the code style only.

[rudolfgrauberger]

@ivangalkin Thank you very much for your feedback/review. I am currently in the exam phase and will answer/work on the points immediately afterwards.

[ivangalkin]

@rudolfgrauberger there is no hurry, good luck with your exams

[rudolfgrauberger]

@ivangalkin Thank you :-)

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ivangalkin)

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 75 at r2 (raw file):

Previously, ivangalkin wrote…

softly.assertThat(includes.contains("...")).isTrue();

here and below should be

softly.assertThat(includes).contains("...");

Assertion written in this way will make the error message much more informative

Thank you. You're right, this provides better information. I'll change this.

Should I just make a new commit after that and push it?

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 90 at r2 (raw file):

Previously, ivangalkin wrote…

We can test this at the moment only if we check how the relative include paths translated to absolute paths

not sure, that I understand the comment. Could you please explain?

This class (CxxVCppBuildLogParser) handle currently the hole process (parses log file, extracts DEFINES, INCLUES, sets default values, resolves path). It is not possible to read out the found project file during this time, because it can change (one log file can build several projects).

Whether a project file was found in the log file can currently only be determined by the fact that the relative INCLUDE paths were correctly resolved relative to the project file. So we need to look at the results of the INCLUDE path resolution to know if the project file was correctly found, extracted and used.

I hope that was a little more understandable. Sorry. I don't speak English very well.

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 125 at r2 (raw file):

Previously, ivangalkin wrote…

 }

EOF looks odd. File encoding is not UTF-8?

It should be UTF8. I think it's because I didn't leave a blank line at the end of the file.

I'll add a blank line at the end of the file (I saw that the other files have this)

---

cxx-squid/src/test/resources/logfile/vc++2017-de.txt, line 1 at r2 (raw file):

• Is this large file really necessary?

I wanted to show that the regular expression has no false positive (both with the current log file vc++13.txt and with the new German log file). But if it is only about the language independent recognition of the project file, then you don't need the whole file.

• Does it provides some exemplary or corner cases?

What does it refer to? The large log file or my change as such.

• Does the test above validates some newly implemented feature? If yes, this is not obvious

It should validate that the project file can now be extracted regardless of the language of the log file.

I first added a test (shouldExtractProjectFileFromEnglishVCpp2013Log) to make sure that after my change the project file can still be extracted from the English log file.

Then I implemented my change and was able to make sure that everything worked as before.

Finally I added the German log file and the test (shouldExtractProjectFileFromGermanVCpp2017Log).

• Could we use sonar-cxx/cxx-sensors/src/test/resources/org/sonar/cxx/sensors/reports-project/compiler-reports/VC-report.vclog instead? could we extend the existing log instead of adding a new one?

I would say that maintaining a file that covers all combinations of a log file will hardly be possible. But at the moment I don't know how such a file could look like. For example, to support multilingual log files, you would have to insert the information redundantly into the log file. But in real log files there will never be this combination...

It is also about different tasks:

• The sensor is about reading the warnings and preparing them for metrics. It knows nothing about projects, includes etc.
• Squid is more about preparing the information. So to extract as much information as possible from the log file and to simplify or reduce the configuration (in sonar-project.properties).

In general I would say that there should be only one place in the optimum which has the responsibility (the reading/extracting of the log file). Then it would also be easier to write tests for it and construct the cases.

I agree, that the test has to validate some realistic scenario. But how can anybody review the correctness of your test if its input contains > 1500 lines. I would minimize the input and emphasize the subject of the test.

I see the problem. How many lines should I reduce the log file to? To pass this test you need about 10 lines.

Should I leave the log file cxx-squid/src/test/resources/logfile/vc++13.txt (was there before) as it was before? There is a test (CxxConfigurationTest) that checks the possible information for the (sonar-project.properties) to be extracted.

Or should I extend the test so that it also checks the new (German) log file? But then I can't reduce the German log file that much.

[ivangalkin]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ivangalkin and @rudolfgrauberger)

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 75 at r2 (raw file):

Previously, rudolfgrauberger (Rudolf Grauberger) wrote…

Thank you. You're right, this provides better information. I'll change this.

Should I just make a new commit after that and push it?

please feel free to make a new commit

---

cxx-squid/src/test/resources/logfile/vc++2017-de.txt, line 1 at r2 (raw file):

Previously, rudolfgrauberger (Rudolf Grauberger) wrote…

• Is this large file really necessary?

I wanted to show that the regular expression has no false positive (both with the current log file vc++13.txt and with the new German log file). But if it is only about the language independent recognition of the project file, then you don't need the whole file.

• Does it provides some exemplary or corner cases?

What does it refer to? The large log file or my change as such.

• Does the test above validates some newly implemented feature? If yes, this is not obvious

It should validate that the project file can now be extracted regardless of the language of the log file.

I first added a test (shouldExtractProjectFileFromEnglishVCpp2013Log) to make sure that after my change the project file can still be extracted from the English log file.

Then I implemented my change and was able to make sure that everything worked as before.

Finally I added the German log file and the test (shouldExtractProjectFileFromGermanVCpp2017Log).

• Could we use sonar-cxx/cxx-sensors/src/test/resources/org/sonar/cxx/sensors/reports-project/compiler-reports/VC-report.vclog instead? could we extend the existing log instead of adding a new one?

I would say that maintaining a file that covers all combinations of a log file will hardly be possible. But at the moment I don't know how such a file could look like. For example, to support multilingual log files, you would have to insert the information redundantly into the log file. But in real log files there will never be this combination...

It is also about different tasks:

• The sensor is about reading the warnings and preparing them for metrics. It knows nothing about projects, includes etc.
• Squid is more about preparing the information. So to extract as much information as possible from the log file and to simplify or reduce the configuration (in sonar-project.properties).

In general I would say that there should be only one place in the optimum which has the responsibility (the reading/extracting of the log file). Then it would also be easier to write tests for it and construct the cases.

I agree, that the test has to validate some realistic scenario. But how can anybody review the correctness of your test if its input contains > 1500 lines. I would minimize the input and emphasize the subject of the test.

I see the problem. How many lines should I reduce the log file to? To pass this test you need about 10 lines.

Should I leave the log file cxx-squid/src/test/resources/logfile/vc++13.txt (was there before) as it was before? There is a test (CxxConfigurationTest) that checks the possible information for the (sonar-project.properties) to be extracted.

Or should I extend the test so that it also checks the new (German) log file? But then I can't reduce the German log file that much.

Ok.

1. The file should be as large as necessary to cover the tested functionality. So some dummy project with (for example) 3 compile units (CPP files), the different types of includes (""-include, <>-include etc) and the different types of defines (-Dstuff, -Dstuff=stuff, -Dstuff(x,y)=(x)+(y)) etc. would be more than enough IMHO.

2. If you want to validate that your parsing is language-independent you'll need the compiler logs of the same project with different language settings. So basically you use your first test (see above) as a reference. The second test parses the German log. Afterwards it asserts that the set of source files, the set of includes and the set of defines are equal to the corresponding sets of the English log.

[guwirth]

@rudolfgrauberger

to get the project file indenpendently of the language of the log file.

Solutions makes it more generic. Think in the past it was no issue because most developer are using an Englisch VS.

Could we use sonar-cxx/cxx-sensors/src/test/resources/org/sonar/cxx/sensors/reports-project/compiler-reports/VC-report.vclog instead? could we extend the existing log instead of adding a new one?

Think it's ok to use two files German/English. But would also shorten it a little bit to the relevant parts.

[rudolfgrauberger]

Ok.

1. The file should be as large as necessary to cover the tested functionality. So some dummy project with (for example) 3 compile units (CPP files), the different types of includes (""-include, <>-include etc) and the different types of defines (-Dstuff, -Dstuff=stuff, -Dstuff(x,y)=(x)+(y)) etc. would be more than enough IMHO.
2. If you want to validate that your parsing is language-independent you'll need the compiler logs of the same project with different language settings. So basically you use your first test (see above) as a reference. The second test parses the German log. Afterwards it asserts that the set of source files, the set of includes and the set of defines are equal to the corresponding sets of the English log.

That sounds good. I'll remove the big log file (vc++2017-de.txt) and create a new and smaller project file for the log file. In this project I will set the different cases for include specifications (relative and absolute paths, with and without quotes) and definitions.

Then I will create a German and an English log file from this project and reduce it to the necessary size.

@ivangalkin and @guwirth Is that all right? Should I do all that in this branch and then just make a new commit?

Solutions makes it more generic. Think in the past it was no issue because most developer are using an Englisch VS.

Could be or most of them didn't use this feature at all and have manually specified all definitions and includes in the properties file (at least I've seen this in some projects before), but that doesn't really matter. :-)

[rudolfgrauberger]

@guwirth Can you restart the build on travis-ci manually, please? The one error is not my fault...

[guwirth]

@rudolfgrauberger looks good from my side. Thx for providing this.

[ivangalkin]

Reviewed 2 of 3 files at r3, 4 of 5 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ivangalkin and @rudolfgrauberger)

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 52 at r4 (raw file):

shouldTranslateRelativeIncludesRelativeToProjectFolderFromDetailedReferenceLog

the naming is a bit long IMHO :-) I would shorten it and add a java doc comment if necessary. Same for all remaining test-cases.

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 109 at r4 (raw file):

        SoftAssertions softly = new SoftAssertions();

        List<String> includes = uniqueIncludes.get(UNIQUE_FILE);

there is nothing to assert as far as I see; just

return uniqueIncludes.get(UNIQUE_FILE);

[ivangalkin]

@rudolfgrauberger I like the tests much more! Thank you. There are still some little thing to improve, but my comments are minor.

[ivangalkin]

@guwirth @rudolfgrauberger BTW there were some important discussion in #1678
As far as I get it @rudolfgrauberger suggested to improve the regular expressions.

Shouldn't we fix them?

[rudolfgrauberger]

Reviewable status: 5 of 6 files reviewed, 5 unresolved discussions (waiting on @ivangalkin)

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 125 at r2 (raw file):

Previously, rudolfgrauberger (Rudolf Grauberger) wrote…

It should be UTF8. I think it's because I didn't leave a blank line at the end of the file.

I'll add a blank line at the end of the file (I saw that the other files have this)

Done.

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 52 at r4 (raw file):

CxxVCppBuildLogParserTest

I have shortened the names and described more about it in the class comment.

The naming convention now matches the style.

"Cxx VCpp BuildLog Parser Test relative Includes From Reference Log"

I hope that's okay.

---

cxx-squid/src/test/java/org/sonar/cxx/CxxVCppBuildLogParserTest.java, line 109 at r4 (raw file):

Previously, ivangalkin wrote…

        SoftAssertions softly = new SoftAssertions();

        List<String> includes = uniqueIncludes.get(UNIQUE_FILE);

there is nothing to assert as far as I see; just

return uniqueIncludes.get(UNIQUE_FILE);

Done.

[rudolfgrauberger]

Error on appveyor

git clone -q https://github.com/SonarOpenCommunity/sonar-cxx.git C:\projects\sonar-cxx
fatal: unable to access 'https://github.com/SonarOpenCommunity/sonar-cxx.git/': Could not resolve host: github.com

@guwirth, @ivangalkin Also this error is not related to my changes. Can you please repeat/restart the build?

[rudolfgrauberger]

@guwirth @rudolfgrauberger BTW there were some important discussion in #1678
As far as I get it @rudolfgrauberger suggested to improve the regular expressions.

Shouldn't we fix them?

Should I create a separate issue for this discussion?

[guwirth]

@rudolfgrauberger I merge it. Let’s see if problem with AppVeyor is still there.