-
Notifications
You must be signed in to change notification settings - Fork 363
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
Add coverage sensor 'Testwell CTC++' for textural reports #1486
Conversation
@rufinio thx for providing this extension. Could you provide some information please.
Did not have the time to do a code review yet. Points should work:
|
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.
Very nice to add support for another commercial coverage tool 👍
sensor = new CxxCoverageSensor(new CxxCoverageCache(), language, context); | ||
sensor.execute(context); | ||
|
||
assertThat(context.lineHits("ProjectKey:HGBuildNumberLookup.cpp", 42)).isEqualTo(10); |
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.
Please use one assertThat statement per test. You can also use SoftAssertions (see http://joel-costigliola.github.io/assertj/core/api/org/assertj/core/api/SoftAssertions.html)
sensor = new CxxCoverageSensor(new CxxCoverageCache(), language, context); | ||
sensor.execute(context); | ||
|
||
assertThat(context.lineHits("ProjectKey:test-wildmatch.c", 3)).isEqualTo(209); |
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.
Please use one assertThat statement per test. You can also use SoftAssertions (see http://joel-costigliola.github.io/assertj/core/api/org/assertj/core/api/SoftAssertions.html)
} | ||
} | ||
|
||
private void addEachLine(CoverageMeasures coverageMeasures) { |
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.
Please reduce complexity of this method (e.g. extract methods). The Cognitive Complexity of this method is 57.
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 you recommend a tool for Cognitive Complexity?
Which value range is good?
if((new File(filename)).isAbsolute() == false) { | ||
normalFilename = FilenameUtils.normalize("./" + filename); | ||
} | ||
else { |
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.
Style: Move this "else" on the same line that the previous closing curly brace (4 times in this file)
Please align with the current style.
A link to the tool page.https://www.verifysoft.com/en_ctcpp.html How to call / use the tool to generate a report for the cxx plugin?Adapted from Testwell CTC++ User's Guid (Basic Use of CTC++) Wondering why you are parsing an ASCII report? Does the tool not provide XML?Testwell CTC++ also provides a XML output as well as JSON output. I mainly use TXT and therefore I know better about it than XML. Handle empty reports.Tested by Test shouldConsumeEmptyReport Support of sonar.cxx.errorRecoveryEnabledI wonder if that is applicable for this extension: In my optinion invalid file references or line numbers has to be handled by calling layer (CxxCoverageSensor). |
@rufinio thx for your answer. Which version of the tool are you using / testing? What should we write in the docu? Supporting version x and later... |
@rufinio found also this page http://www.testwell.fi/ctcdesc.html. Is the tool from Testwell or Verisoft? |
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.
Looks good. Only minor findings.
*/ | ||
@Override | ||
public void processReport(final SensorContext context, File report, final Map<String, CoverageMeasures> coverageData) { | ||
LOG.debug("Parsing 'Testwell CTC++' format"); |
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 make clear that ‚only‘ textual format is supported:
Parsing 'Testwell CTC++' textual format
@@ -67,6 +67,7 @@ public CxxCoverageSensor(CxxCoverageCache cache, CxxLanguage language, SensorCon | |||
parsers.add(new CoberturaParser()); | |||
parsers.add(new BullseyeParser()); | |||
parsers.add(new VisualStudioParser()); | |||
parsers.add(new TestwellCtcParser()); |
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.
Maybe better TestwellCtcTxtParser?
@rufinio Technical feedback: I am not sure, if your parser fits into the existing framework. Please see the @guwirth w.r.t. #1465 and the generic coverage format, wouldn't it be more foresighted to create an external (?) converter CTC++ -> XML and import the coverage by means of |
one more side remark: TestwellCtcParser.java has common parts with https://github.com/Londran/sonar-ctc/blob/master/src/main/java/org/sonar/plugins/ctc/api/parser/CtcTextParser.java (among others some logging messages are 100% identical). The original file is copyrighted
Not sure if you can just skip the original copyright of LGPL3 code. Please double-check. |
Source is LGPL and as long derivative work is again under license LGPL this should be no problem. But for later documentation it makes sense to add a comment below our header with a link to the original source. |
Testwell CTC++PrerequisitesTestwell CTC++ Version 7.2 and later. 1. Generate instrumented executableSuppose we have the following source files that form a complete program: prime.c io.c calc.c. This program can be compiled and linked in many ways, one way being the following 2. Generate coverage reportAfter this test run we notice that the file MON.dat has born in the current directory (same directory as the symbolfile MON.sym was created to). It is a datafile, containing the collected execution counters when the code in the instrumented files was executed. 3. Read coverage file with SonarQubeTo get the results of our test to sonarqube we add the following line to configuration file sonar-project.properties:
Verifysoft Technology GmbH (Offenburg/Germany) has acquired all intellectual property rights for the software test and analysis tools of Testwell (Finland). @ivangalkin: |
…port can have TXT, XML or JSON format. This parser is aligned to TXT format.
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.
some minor comments w.r.t. to the style etc
FILE_COND("^\\Q***TER\\E +\\d+ % \\( *(\\d+)/ *(\\d+)\\) of FILE (?:.*)$"), | ||
FILE_STMT("^ {6} +\\d+ % \\( *(\\d+)/ *(\\d+)\\) statement.*$"); | ||
|
||
public static final Pattern REPORT_HEADER = Pattern.compile(MON_SYM.patternString + "\\s+" + MON_DAT.patternString |
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.
here and below the joining can be simplified IMHO
String.join("\s+", MON_SYM.patternString, MON_DAT.patternString, LIS_DTE.patternString, COV_VIW.patternString);
public static final Pattern REPORT_FOOTER = Pattern.compile(SRC_FLS.patternString + "\\s+" + HDR_EXT.patternString + "\\s+" | ||
+ FKT_EXT.patternString + "\\s*" + SRC_LNS.patternString, MULTILINE); | ||
public static final Pattern FILE_HEADER = Pattern.compile(FILE_MONI.patternString + "\\s+" + FILE_INST.patternString, MULTILINE); | ||
public static final Pattern SECTION_SEP = compile("^-{77}|={77}$", MULTILINE); |
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.
here and below: non-consistent compile() invocation: Pattern.compile() vs just compile()
I personally would prefer not to import like import static java.util.regex.Pattern.compile;
but that's a matter of taste; the code just has to be consistent
private static final Logger LOG = Loggers.get(TestwellCtcTxtParser.class); | ||
|
||
private Scanner scanner; | ||
private Matcher matcher; |
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.
keeping Matcher object as a state doesn't look correct to me. I believe it wold be easier to read and to understand if Matcher will be created in scope of each function. Especially because of permanent calls like matcher.reset() or matcher.usePattern()
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.
Using matcher.reset() is better for performance at the cost of being less readable. In the case of a loop it is one less object to create in each loop iteration, and correspondingly one less object to be garbage collected per loop iteration.
After I moved matcher object in scope of each method I saw in my tests with report_big.txt about 5% decrease in performance. Hope that is compensated by the better readability.
|
||
private Scanner scanner; | ||
private Matcher matcher; | ||
private State state; |
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.
same here: having a 'state' only makes the logic more complex IMHO
the main parsing loop is just something like that:
if ( parseReportHead() )
{
while( parseUnit(coverageData) )
{
}
}
where each function returns true if parsing was successful
private void parseFileUnit(final Map<String, CoverageMeasures> coverageData) { | ||
LOG.debug("Parsing file section..."); | ||
|
||
String normalFilename; |
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.
here and below: there is no need to declare variable in advance, it's not C89
int lineHitsFalse; | ||
boolean conditionDetected; | ||
|
||
lineIdPrev = 0; |
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.
same here and below: please combine declaration and initialization (although '0' and 'false' are actually set by default, but maybe it is more readable)
|
||
private void addEachLine(CoverageMeasures coverageMeasures) { | ||
|
||
int lineIdCur; |
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.
int lineIdCur = Integer.parseInt(matcher.group(LINE_NR_GROUP));
belong into the do-loop
please double-check the following variable declaration; the scope of some of them must be reduced;
only loop-invariants or some states (accumulators) should be declared here
} else { | ||
// multicondition | ||
|
||
if (conditionDetected == true) { |
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.
if (conditionDetected) {
} | ||
|
||
private void setLinehits(CoverageMeasures coverageMeasures, int lineIdPrev, int lineIdCur, int lineHits) { | ||
int lineIdNext; |
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.
please reduce the scope
if (lineIdPrev > 0) {
int lineIdNext = lineIdPrev + 1;
while (lineIdNext < lineIdCur) {
....
private final MapSettings settings = new MapSettings(); | ||
|
||
@Before | ||
public void setUp() { |
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.
It is very error-prone to share mutable state between test cases
maybe it's convenient (?) to declare ...
- private CxxCoverageSensor sensor;
- private DefaultFileSystem fs;
- private SensorContextTester context;
... and permanently reset them in @Before
...
but sharing of...
- private final MapSettings settings = new MapSettings();
... is just wrong IMHO
private static MapSettings singletonMapSettings( String key, String value )
{
MapSettings settings = new MapSettings();
settings.set( key, value );
return settings;
}
@Test
public void myWhateverTest() {
MapSettings settings = singletonMapSettings( "some key", "some value );
}
P. S. I grepped in other tests for settings
and these pattern seems to be common (which still doesn't make it correct). Feel free to ignore my comment.
@ivangalkin maybe we should fix this in an extra refactoring task for all unit tests? |
@rufinio do you have time to fix the review comments from @ivangalkin? |
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.
Looks good, ready to merge.
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.
Thank you for applying of the review comments. The current ones are less important: The one with try-with-resources or the scope of Matcher headerMatcher
could be still improved IMHO, but the remaining comments are about style. You decide. Great job BTW
@@ -79,113 +71,88 @@ public TestwellCtcTxtParser() { | |||
@Override | |||
public void processReport(final SensorContext context, File report, final Map<String, CoverageMeasures> coverageData) { | |||
LOG.debug("Parsing 'Testwell CTC++' textual format"); | |||
|
|||
|
|||
try { | |||
this.scanner = new Scanner(report).useDelimiter(SECTION_SEP); |
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 missed this during the previous comment: in order to avoid this.scanner.close() calls (e. g. in case of exception), one could use so called "try with resources".
try ( this.scanner = new Scanner(report).useDelimiter(SECTION_SEP) )
{
...
}
catch ( ... )
{
}
Since Scanner is AutoCloseable it will be automatically closed when control flow lefts the try scope (see documentation)
} else { | ||
state = State.END; | ||
scanner.close(); |
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.
scanner.close()
will be not necessary if try-with-resources will be used
while (!matcher.reset(scanner.next()).usePattern(FILE_RESULT).find()) { | ||
parseLineSection(coverageMeasures); | ||
String nextLine = scanner.next(); | ||
while (!FILE_RESULT.matcher(nextLine).find()) { |
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.
looks like a classic use-case for a for loop
to me:
for (String nextLine = scanner.next(); FILE_RESULT.matcher(nextLine).find(); nextLine = scanner.next())
{
parseLineSection(coverageMeasures, nextLine);
}
... but it's a matter of taste
filename = matcher.group(1); | ||
if((new File(filename)).isAbsolute() == false) { | ||
String filename = headerMatcher.group(1); | ||
if (!(new File(filename)).isAbsolute()) { |
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.
parenthesis are not necessary if (!new File(filename).isAbsolute()) {
@rufinio for me the code is ok. If you like to work-in the last points from @ivangalkin tell me. |
Yes, I'd like to apply these last points. |
… to for loop. Inverted if statement.
@rufinio thanks a lot for your contribution! |
This change is