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

Surefire-report incompatible with Sonar #322

Closed
pettermahlen opened this Issue May 17, 2012 · 17 comments

Comments

Projects
None yet
7 participants
@pettermahlen

pettermahlen commented May 17, 2012

When running a project with two scenarios in the same feature file, and where both scenarios partially use the same steps, we get the following error from Sonar:

Caused by: org.sonar.api.utils.SonarException: Can not add twice the same measure on [default].Given a UOR containing: org.sonar.api.measures.Measure@3f58cd42[

The corresponding surefire-reports/XX.xml file contains the following testcase results (I stripped off the leading < to get them to render):

testcase time="0" classname="Given a UOR containing " name="Given a UOR containing "/>
testcase time="0" classname="And an input file with the following contents: " name="And an input file with the following contents: "/>
testcase time="0.001" classname="And the OID lookup service responds: " name="And the OID lookup service responds: "/>
testcase time="0" classname="When the DI system receives an ingestion request " name="When the DI system receives an ingestion request "/>
testcase time="0" classname="Then the job status is eventually \"DONE\" " name="Then the job status is eventually \"DONE\" "/>
testcase time="0" classname="And the Unassembled Offer Store contains: " name="And the Unassembled Offer Store contains: "/>
testcase time="0" classname="And the Data Enrichment system receives the following messages: " name="And the Data Enrichment system receives the following messages: "/>
testcase time="0.001" classname="Scenario: Message flagged in progress\u002C but time not expired " name="Scenario: Message flagged in progress\u002C but time not expired "/>
testcase time="0" classname="Given a UOR containing " name="Given a UOR containing "/>
testcase time="0" classname="And an input file with the following contents: " name="And an input file with the following contents: "/>
testcase time="0" classname="And the OID lookup service responds: " name="And the OID lookup service responds: "/>
testcase time="0" classname="When the DI system receives an ingestion request " name="When the DI system receives an ingestion request "/>
testcase time="0" classname="Then the job status is eventually \"DONE\" " name="Then the job status is eventually \"DONE\" "/>
testcase time="0" classname="And the Unassembled Offer Store contains: " name="And the Unassembled Offer Store contains: "/>
testcase time="0" classname="And the Data Enrichment system receives the following messages: " name="And the Data Enrichment system receives the following messages: "/>
testcase time="0" classname="Scenario: Message flagged in progress\u002C time has expired " name="Scenario: Message flagged in progress\u002C time has expired"/>

It seems like Sonar requires the class+test names to be unique within reports. I'm not sure if the problem is more a Sonar issue or more a Cucumber issue but it does seem like being able to distinguish the same step run in different scenarios might be useful.

@aslakhellesoy

This comment has been minimized.

Show comment
Hide comment
@aslakhellesoy

aslakhellesoy May 17, 2012

Contributor

As I have mentioned in cucumber-attic/cucumber-tck#14 (moved to cucumber/cucumber-js#63) I think it would make sense to generate the JUnit XML report from a separate command line program:

cat report.json | cucumber-json-to-xml > report.xml

All Cucumber implementations have buggy and slightly different JUnit XML support, and this would move it to one place.

Contributor

aslakhellesoy commented May 17, 2012

As I have mentioned in cucumber-attic/cucumber-tck#14 (moved to cucumber/cucumber-js#63) I think it would make sense to generate the JUnit XML report from a separate command line program:

cat report.json | cucumber-json-to-xml > report.xml

All Cucumber implementations have buggy and slightly different JUnit XML support, and this would move it to one place.

@pettermahlen

This comment has been minimized.

Show comment
Hide comment
@pettermahlen

pettermahlen May 17, 2012

Hm, OK - I've just been running cucumber-java via Maven/surefire/cucumber-junit, and there's no report.json file being generated AFAICT. If my quick glance through the code is right, I would guess that the names are generated by the cucumber.junit.DescriptionFactory, which seems to use something that might work better with the 4.11 snapshot version of JUnit? I don't quite get it, but maybe I don't have to. :)

Anyway, if a command-line utility is the preferred solution, how would it plug into the Maven lifecycle? To me, it feels like it would be very useful to keep the native JUnit support when actually running JUnit, but perhaps a good solution is to wrap that native support in a command-line utility when other testing tools want to generate JUnit-formatted output?

pettermahlen commented May 17, 2012

Hm, OK - I've just been running cucumber-java via Maven/surefire/cucumber-junit, and there's no report.json file being generated AFAICT. If my quick glance through the code is right, I would guess that the names are generated by the cucumber.junit.DescriptionFactory, which seems to use something that might work better with the 4.11 snapshot version of JUnit? I don't quite get it, but maybe I don't have to. :)

Anyway, if a command-line utility is the preferred solution, how would it plug into the Maven lifecycle? To me, it feels like it would be very useful to keep the native JUnit support when actually running JUnit, but perhaps a good solution is to wrap that native support in a command-line utility when other testing tools want to generate JUnit-formatted output?

@aslakhellesoy

This comment has been minimized.

Show comment
Hide comment
@aslakhellesoy

aslakhellesoy May 17, 2012

Contributor

Did you --format json:some/file.json ?

I'm no Maven expert, but running an arbitrary command after tests is possible. Just read up on the maven docs.
We could easily wrap a command line tool in a junit formatter as well (and shell out) to keep things simpler.

Also see the email I sent to the list just now.

Contributor

aslakhellesoy commented May 17, 2012

Did you --format json:some/file.json ?

I'm no Maven expert, but running an arbitrary command after tests is possible. Just read up on the maven docs.
We could easily wrap a command line tool in a junit formatter as well (and shell out) to keep things simpler.

Also see the email I sent to the list just now.

@mikquinlan

This comment has been minimized.

Show comment
Hide comment
@mikquinlan

mikquinlan May 19, 2012

Hi

We ran with

@Cucumber.Options(format = "junit:target/surefire-reports/TEST-com.mycompany.imp.di.acceptance.RunDiIntegrationTest.xml")

in the hope that the report would be generated and replace the surefire generated one.

The report is indeed generated (we see it when we run the Cucumber tests on their own), but at the end of the build cycle, the surefire version is still in surefire-reports. It is like it overwrites the Cucumber generated report. Changing the name of the output file, above, to ...RunDiIntegrationTest2.xml results in both the surefire version and the Cucumber version being generated in the surefire-reports directory.

I am investigating now and was wondering if there was a way to prevent this from happening? It's most probably a build lifecycle issue - surefire collating its results and writing them out at the end of the test, thereby overwriting what Cucumber has generated at the end of its single test (as Maven sees it) execution.

mikquinlan commented May 19, 2012

Hi

We ran with

@Cucumber.Options(format = "junit:target/surefire-reports/TEST-com.mycompany.imp.di.acceptance.RunDiIntegrationTest.xml")

in the hope that the report would be generated and replace the surefire generated one.

The report is indeed generated (we see it when we run the Cucumber tests on their own), but at the end of the build cycle, the surefire version is still in surefire-reports. It is like it overwrites the Cucumber generated report. Changing the name of the output file, above, to ...RunDiIntegrationTest2.xml results in both the surefire version and the Cucumber version being generated in the surefire-reports directory.

I am investigating now and was wondering if there was a way to prevent this from happening? It's most probably a build lifecycle issue - surefire collating its results and writing them out at the end of the test, thereby overwriting what Cucumber has generated at the end of its single test (as Maven sees it) execution.

@pettermahlen

This comment has been minimized.

Show comment
Hide comment
@pettermahlen

pettermahlen May 21, 2012

So, I've spent a few more hours debugging the test runner and I think I've figured out, more or less, what this issue is about.

The root of the problem seems to be that the test case name in the output generated by Surefire (the Maven plugin that runs tests) is the same as the step name, where instead, the class name should correspond to the Scenario and the 'name' should be the step:

testcase time="0" classname="Given a UOR containing " name="Given a UOR containing "/>

is wrong, but

testcase time="0" classname="Scenario: Message flagged in progress\u002C time has expired" name="Given a UOR containing "/>

is right.

This boils down to a mismatch between the Cucumber-JUnit ExecutionUnitRunner and the JUnit Description class. The ExecutionUnitRunner uses (in a roundabout way) a static createSuiteDescription factory method for every description rather than the createTestDescription method that would have been appropriate for create step descriptions. The reason, I'm sure, is that in this case, we don't have an actual Java class that the test case lives in, but rather a Scenario.

There's some code in Description that uses a regexp to split out the class name from the method/test case name when reporting, and that code returns the whole description if the regexp doesn't match - this explains why we're getting the step description both for the class name and the test case name above.

    public String getClassName() {
        Matcher matcher= methodStringMatcher();
        return matcher.matches()
            ? matcher.group(2)
            : toString();
    }

    /**
     * @return If this describes a method invocation, 
     * the name of the method (or null if not)
     */
    public String getMethodName() {
        return parseMethod();
    }

    private String parseMethod() {
        Matcher matcher= methodStringMatcher();
        if (matcher.matches())
            return matcher.group(1);
        return null;
    }

    private Matcher methodStringMatcher() {
        return Pattern.compile("(.*)\\((.*)\\)").matcher(toString());
    }

I can see a couple of possible ways to fix the problem. Either change the ExecutionUnitRunner like this:

    @Override
    protected Description describeChild(Step step) {
        Description description = stepDescriptions.get(step);
        if (description == null) {
            description = createDescription(String.format("%s(%s)", step.getKeyword() + step.getName(), this.description), step);
            stepDescriptions.put(step, description);
        }
        return description;
    }

Or, as a second option, change JUnit so that Description doesn't require an explicit Class to instantiate a Test-level description, and make Cucumber-JUnit use a method that allows specifying the class name as a string.

The first solution has a couple of problems:
First, the 'uniqueness' code introduced to solve this issue #225. It appends spaces at the end of descriptions, meaning that the regexp doesn't match and therefore class name parsing doesn't work - here's what the output looks like when using the code above:

testcase time="0.001" classname="Given a UOR containing(Scenario: Message flagged in progress\u002C but time not expired  )   " name="Given a UOR containing"/>

Second, it uses knowledge that should be internal to the Description class - namely that it internally changes a combination of class name + test case name into a "%s(%s)".

This works in the sense that Sonar accepts it, but the output is ugly. We're currently running this solution in-house, as an emergency patch to keep our builds running. The code is available here: https://github.com/pettermahlen/cucumber-jvm/tree/cucumber-jvm-322

I think the best solution is to change Description so that it stores this information in two separate strings and simply returns them (rather than going the roundabout way of regexp matching), and to add a createTestDescription factory method that takes two strings rather than a class and a string. This factory method should be used when describing steps, rather than the createSuiteDescription.

I don't think it's reasonable to not produce correct output via the standard JUnit mechanisms, so I think this is an actual issue with Cucumber-JUnit.

Does that seem to make sense? Should we try to raise an issue with JUnit?

pettermahlen commented May 21, 2012

So, I've spent a few more hours debugging the test runner and I think I've figured out, more or less, what this issue is about.

The root of the problem seems to be that the test case name in the output generated by Surefire (the Maven plugin that runs tests) is the same as the step name, where instead, the class name should correspond to the Scenario and the 'name' should be the step:

testcase time="0" classname="Given a UOR containing " name="Given a UOR containing "/>

is wrong, but

testcase time="0" classname="Scenario: Message flagged in progress\u002C time has expired" name="Given a UOR containing "/>

is right.

This boils down to a mismatch between the Cucumber-JUnit ExecutionUnitRunner and the JUnit Description class. The ExecutionUnitRunner uses (in a roundabout way) a static createSuiteDescription factory method for every description rather than the createTestDescription method that would have been appropriate for create step descriptions. The reason, I'm sure, is that in this case, we don't have an actual Java class that the test case lives in, but rather a Scenario.

There's some code in Description that uses a regexp to split out the class name from the method/test case name when reporting, and that code returns the whole description if the regexp doesn't match - this explains why we're getting the step description both for the class name and the test case name above.

    public String getClassName() {
        Matcher matcher= methodStringMatcher();
        return matcher.matches()
            ? matcher.group(2)
            : toString();
    }

    /**
     * @return If this describes a method invocation, 
     * the name of the method (or null if not)
     */
    public String getMethodName() {
        return parseMethod();
    }

    private String parseMethod() {
        Matcher matcher= methodStringMatcher();
        if (matcher.matches())
            return matcher.group(1);
        return null;
    }

    private Matcher methodStringMatcher() {
        return Pattern.compile("(.*)\\((.*)\\)").matcher(toString());
    }

I can see a couple of possible ways to fix the problem. Either change the ExecutionUnitRunner like this:

    @Override
    protected Description describeChild(Step step) {
        Description description = stepDescriptions.get(step);
        if (description == null) {
            description = createDescription(String.format("%s(%s)", step.getKeyword() + step.getName(), this.description), step);
            stepDescriptions.put(step, description);
        }
        return description;
    }

Or, as a second option, change JUnit so that Description doesn't require an explicit Class to instantiate a Test-level description, and make Cucumber-JUnit use a method that allows specifying the class name as a string.

The first solution has a couple of problems:
First, the 'uniqueness' code introduced to solve this issue #225. It appends spaces at the end of descriptions, meaning that the regexp doesn't match and therefore class name parsing doesn't work - here's what the output looks like when using the code above:

testcase time="0.001" classname="Given a UOR containing(Scenario: Message flagged in progress\u002C but time not expired  )   " name="Given a UOR containing"/>

Second, it uses knowledge that should be internal to the Description class - namely that it internally changes a combination of class name + test case name into a "%s(%s)".

This works in the sense that Sonar accepts it, but the output is ugly. We're currently running this solution in-house, as an emergency patch to keep our builds running. The code is available here: https://github.com/pettermahlen/cucumber-jvm/tree/cucumber-jvm-322

I think the best solution is to change Description so that it stores this information in two separate strings and simply returns them (rather than going the roundabout way of regexp matching), and to add a createTestDescription factory method that takes two strings rather than a class and a string. This factory method should be used when describing steps, rather than the createSuiteDescription.

I don't think it's reasonable to not produce correct output via the standard JUnit mechanisms, so I think this is an actual issue with Cucumber-JUnit.

Does that seem to make sense? Should we try to raise an issue with JUnit?

@pettermahlen

This comment has been minimized.

Show comment
Hide comment
@pettermahlen

pettermahlen May 25, 2012

@aslakhellesoy - just wanted to see if you have any opinions on this? I'd be willing to drive making a change with JUnit if you think that's the way to go.

pettermahlen commented May 25, 2012

@aslakhellesoy - just wanted to see if you have any opinions on this? I'd be willing to drive making a change with JUnit if you think that's the way to go.

@mattharr

This comment has been minimized.

Show comment
Hide comment
@mattharr

mattharr Jun 4, 2012

Contributor

Hi,

I've been having a play with the Surefire stuff too - due to the parallel running not working (#333). I think there is a bit of an issue with the Description stuff - have you worked through anything with this?

Contributor

mattharr commented Jun 4, 2012

Hi,

I've been having a play with the Surefire stuff too - due to the parallel running not working (#333). I think there is a bit of an issue with the Description stuff - have you worked through anything with this?

@pettermahlen

This comment has been minimized.

Show comment
Hide comment
@pettermahlen

pettermahlen Jun 4, 2012

Without having gone through the code that creates the maps, etc., that are needed in your example, it does sound very much like the same sort of issue and could perhaps be the exact same thing. The JUnit Description class expects a Class and a method name at the individual test case level, and when Cucumber maps steps to individual test cases, it doesn't give the Description the data it expects. As I mentioned above, I think the best solution is to loosen the requirements of the Description class so that can take any two names instead of a Class and a name.

pettermahlen commented Jun 4, 2012

Without having gone through the code that creates the maps, etc., that are needed in your example, it does sound very much like the same sort of issue and could perhaps be the exact same thing. The JUnit Description class expects a Class and a method name at the individual test case level, and when Cucumber maps steps to individual test cases, it doesn't give the Description the data it expects. As I mentioned above, I think the best solution is to loosen the requirements of the Description class so that can take any two names instead of a Class and a name.

@aslakhellesoy

This comment has been minimized.

Show comment
Hide comment
@aslakhellesoy

aslakhellesoy Jun 4, 2012

Contributor

@pettermahlen my KentBeck/junit#405 patch has been merged into JUnit. It's not released yet, but when the next release of JUnit is out we can remove the code that appends spaces to hack around the current JUnit <= 4.10 limitations.

Could you try with your own build of JUnit master and your proposed change to describeChild?

Contributor

aslakhellesoy commented Jun 4, 2012

@pettermahlen my KentBeck/junit#405 patch has been merged into JUnit. It's not released yet, but when the next release of JUnit is out we can remove the code that appends spaces to hack around the current JUnit <= 4.10 limitations.

Could you try with your own build of JUnit master and your proposed change to describeChild?

@pettermahlen

This comment has been minimized.

Show comment
Hide comment
@pettermahlen

pettermahlen Jun 4, 2012

I've tried this out, and it looks mostly fine, although two issues remain:

  1. Step doesn't implement Serializable, so I had to modify JUnit to use Object instead of Serializable in the new constructor+factory method that you added in issue #405. I think Gherkin should be fixed so that Step implements Serializable, but it was too hard to build Gherkin locally (too many dependencies), so I never made that change myself.
  2. I think that the fix may not be quite enough, since most likely Sonar will still have problems with Scenario Outlines and Examples. At least I believe that 'class name' + 'method name' combinations need to be unique for Sonar, and I believe that they are not guaranteed to be so in the Scenario Outline + Examples combination. Is that so? Is it perhaps useful to be able to easily distinguish different scenario executions in other contexts, so that it might make sense to add some more identifier to the scenario visual name if so, or is that really kludgy?

pettermahlen commented Jun 4, 2012

I've tried this out, and it looks mostly fine, although two issues remain:

  1. Step doesn't implement Serializable, so I had to modify JUnit to use Object instead of Serializable in the new constructor+factory method that you added in issue #405. I think Gherkin should be fixed so that Step implements Serializable, but it was too hard to build Gherkin locally (too many dependencies), so I never made that change myself.
  2. I think that the fix may not be quite enough, since most likely Sonar will still have problems with Scenario Outlines and Examples. At least I believe that 'class name' + 'method name' combinations need to be unique for Sonar, and I believe that they are not guaranteed to be so in the Scenario Outline + Examples combination. Is that so? Is it perhaps useful to be able to easily distinguish different scenario executions in other contexts, so that it might make sense to add some more identifier to the scenario visual name if so, or is that really kludgy?
@aslakhellesoy

This comment has been minimized.

Show comment
Hide comment
@aslakhellesoy

aslakhellesoy Jun 4, 2012

Contributor

Step doesn't implement Serializable

I've added a ticket for that.

I think that the fix may not be quite enough, since most likely Sonar will still have problems with Scenario Outlines and Examples.

Let's see if we can get the simple case to work first - with plain scenarios. I think handling of Scenario Outlines should be handled in a new ticket after the simple case works.

Contributor

aslakhellesoy commented Jun 4, 2012

Step doesn't implement Serializable

I've added a ticket for that.

I think that the fix may not be quite enough, since most likely Sonar will still have problems with Scenario Outlines and Examples.

Let's see if we can get the simple case to work first - with plain scenarios. I think handling of Scenario Outlines should be handled in a new ticket after the simple case works.

@thoraage

This comment has been minimized.

Show comment
Hide comment
@pettermahlen

This comment has been minimized.

Show comment
Hide comment
@pettermahlen

pettermahlen Sep 18, 2012

The JUnit pull request has been accepted for some time, but I've been unable to get any confirmation on when JUnit 4.11 might be out. I guess it's not really worthwhile trying to proceed with this fix until we have a real JUnit version out there.

pettermahlen commented Sep 18, 2012

The JUnit pull request has been accepted for some time, but I've been unable to get any confirmation on when JUnit 4.11 might be out. I guess it's not really worthwhile trying to proceed with this fix until we have a real JUnit version out there.

@nhajratw

This comment has been minimized.

Show comment
Hide comment
@nhajratw

nhajratw Oct 16, 2012

Contributor

junit 4.11-beta1 is out (in maven central at least) -- perhaps this can be revisited now?

Contributor

nhajratw commented Oct 16, 2012

junit 4.11-beta1 is out (in maven central at least) -- perhaps this can be revisited now?

aslakhellesoy added a commit that referenced this issue Oct 24, 2012

Updated to JUnit 4.11. Relates to #255. Might also relate to #322. In…
…telliJ IDEA reports features with Background and Scenario Outline incorrect.y. Might be the case on master too.
@davidsmithgumtree

This comment has been minimized.

Show comment
Hide comment
@davidsmithgumtree

davidsmithgumtree Dec 19, 2012

The fix for naming in surefire reports is in 1.0.15-SNAPSHOT but did not make it into 1.1, why was this?

davidsmithgumtree commented Dec 19, 2012

The fix for naming in surefire reports is in 1.0.15-SNAPSHOT but did not make it into 1.1, why was this?

@aslakhellesoy

This comment has been minimized.

Show comment
Hide comment
@aslakhellesoy

aslakhellesoy Dec 19, 2012

Contributor

@davidsmithgumtree what fix is this? Got a link to the relevant commit?

Contributor

aslakhellesoy commented Dec 19, 2012

@davidsmithgumtree what fix is this? Got a link to the relevant commit?

@davidsmithgumtree

This comment has been minimized.

Show comment
Hide comment
@davidsmithgumtree

davidsmithgumtree Dec 19, 2012

The stuff for dealing with Description in 98d804d

davidsmithgumtree commented Dec 19, 2012

The stuff for dealing with Description in 98d804d

pettermahlen pushed a commit to pettermahlen/cucumber-jvm that referenced this issue Jan 3, 2013

pettermahlen pushed a commit to pettermahlen/cucumber-jvm that referenced this issue Jan 3, 2013

pettermahlen pushed a commit to pettermahlen/cucumber-jvm that referenced this issue Jan 3, 2013

Petter Måhlén
issue #322: fixed the lookup of the createSuiteDescription method in …
…DescriptionFactory and made some classes Serializable

aslakhellesoy added a commit that referenced this issue Jan 12, 2013

Updated to JUnit 4.11. Relates to #255. Might also relate to #322. In…
…telliJ IDEA reports features with Background and Scenario Outline incorrect.y. Might be the case on master too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment