SERG-Delft / jpacman-framework

Pacman-inspired game, for teaching testing purposes.
123 stars 254 forks source link

Remove Cobertura from reporting #78

Closed TimvdLippe closed 7 years ago

TimvdLippe commented 8 years ago

Cobertura is not able to parse any Java 8 statement and Jacoco seems to be a good replacement. Therefore we should remove Cobertura.

However, as @JWGmeligMeyling pointed out, DevHub still uses Cobertura for the warning system. We therefore need to wait till DevHub is updated before removing this plugin.

jwgmeligmeyling commented 8 years ago

Corbertura already supports Java 8?

avandeursen commented 8 years ago

It works with Java 8, but if your code contains a lambda, Cobertura stops with a parse error.

[ERROR] net.sourceforge.cobertura.javancss.parser.ParseException: Encountered " ">" "> "" at line 38, column 28.
Was expecting one of:
    "assert" ...
    "boolean" ...
jwgmeligmeyling commented 8 years ago

Thats only one check using the Javancss parser. I believe that bas been fixed in 2.7: https://github.com/cobertura/cobertura/issues/166

TimvdLippe commented 8 years ago

We are using Cobertura 2.7 at the moment and the issue does not seem to be fixed.

avandeursen commented 8 years ago

Ah, it seems the maven command does show the [ERROR], but then happily and correctly continues to do the coverage analysis:

image

And generates a correct report:

image

jwgmeligmeyling commented 8 years ago

JavaNCSS is a separate report though, that calculates a few code measurements, and as far as I remember, this error does not break the coverage report itself. It only logs a verbose error.

TimvdLippe commented 8 years ago

Loading Cobertura together with Jacoco will result in warnings for Jacoco:

[INFO] --- jacoco-maven-plugin:0.7.6.201602180812:check (default-check) @ jpacman-framework ---
[INFO] Analyzed bundle 'jpacman-framework' with 56 classes
[WARNING] Classes in bundle 'jpacman-framework' do no match with execution data. For report generation the same class files must be used as at runtime.
[WARNING] Execution data for class nl/tudelft/jpacman/level/LevelFactory does not match.
[WARNING] Execution data for class nl/tudelft/jpacman/level/MapParser does not match.

Loading both results in wrong testresults for Jacoco.

jwgmeligmeyling commented 8 years ago

That could be true, as cobertura uses bytecode instrumentation to compute the coverage, resulting in lower coverage at the injected byte code. I hope that this bytecode instrumentation however takes place in the cobertura phase and not in the regular compilation phase.

avandeursen commented 8 years ago

Which mvn target trigger these warnings?

TimvdLippe commented 8 years ago

mvn site triggered them. After the test (but before the Javadoc output) ran you can see the [WARNING] multiple times during several phases of jacoco.

avandeursen commented 8 years ago

OK, I see them now. We should not have both in the pom then it seems.

TimvdLippe commented 7 years ago

@avandeursen This issue is similar to the Hamcrest vs AsertJ. Jacoco is significant more up-to-date and there are also reports of enormous test time improvements: https://twitter.com/rafaelcodes/status/808288098790572033

@JWGmeligMeyling can we make the transition to just Jacoco?

avandeursen commented 7 years ago

Done. Thanks for good discussion.