[GitHub] [ant] mguillem opened a new pull request #121: use displayName instead of legacyReportingName in xml reports

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [ant] mguillem opened a new pull request #121: use displayName instead of legacyReportingName in xml reports

GitBox
mguillem opened a new pull request #121: use displayName instead of legacyReportingName in xml reports
URL: https://github.com/apache/ant/pull/121
 
 
   Allows to have the configured informative test names in the reports instead of the test method signature (like `1: fib(0) = 0`, ..., `7: fib(6) = 8` instead of `fibonacci(int, int)[1]`, ..., `fibonacci(int, int)[7]`.
   
   See discussion in code : https://github.com/apache/ant/commit/5981e1bff1e095bf85ee4d6565bc0c7ff93f2bb6#diff-f985f89ce4779eff2cdcf164d09d5396R246
   
   I was not able to run successfully all tests locally as desired. With some hacks I was able to run `JUnitLauncherTaskTest` with `ant junit-single-test -Djunit.testcase=org.apache.tools.ant.taskdefs.optional.junitlauncher.JUnitLauncherTaskTest` to prepare this patch but can't be sure that the changes don't break something else.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [ant] jaikiran commented on issue #121: use displayName instead of legacyReportingName in xml reports

GitBox
jaikiran commented on issue #121: use displayName instead of legacyReportingName in xml reports
URL: https://github.com/apache/ant/pull/121#issuecomment-580211443
 
 
   Thank you for creating this PR. Looks like our CI hasn't been picking up PR related runs. I'll run some tests manually and see how it goes.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [ant] mguillem commented on issue #121: use displayName instead of legacyReportingName in xml reports

GitBox
In reply to this post by GitBox
mguillem commented on issue #121: use displayName instead of legacyReportingName in xml reports
URL: https://github.com/apache/ant/pull/121#issuecomment-581270795
 
 
   @jaikiran Is this PR ok for you?
   I'm waiting for this to be merged (or rejected) to submit a new PR with some common changes.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [ant] jaikiran commented on issue #121: use displayName instead of legacyReportingName in xml reports

GitBox
In reply to this post by GitBox
jaikiran commented on issue #121: use displayName instead of legacyReportingName in xml reports
URL: https://github.com/apache/ant/pull/121#issuecomment-582732489
 
 
   Hello @mguillem, please give me a few days on this one. I have tested this locally and do see some issues with the changes (not that we won't be able to solve them). However, I am in the middle of few other things these days so in order to get to this code and explain the issue, I will need a few more days. I'm sorry about that.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [ant] jaikiran commented on issue #121: use displayName instead of legacyReportingName in xml reports

GitBox
In reply to this post by GitBox
jaikiran commented on issue #121: use displayName instead of legacyReportingName in xml reports
URL: https://github.com/apache/ant/pull/121#issuecomment-585021713
 
 
   Hello @mguillem, sorry to keep you waiting and thank you for the patience.
   
   Here's more details on why I used `legacyReportingName` when I fixed https://bz.apache.org/bugzilla/show_bug.cgi?id=63680 and why the current proposed change can cause issues.
   
   Please consider the following testcase:
   ```
   package org.myapp;
   
   import org.junit.jupiter.params.ParameterizedTest;
   import org.junit.jupiter.params.provider.ValueSource;
   import org.junit.jupiter.api.RepeatedTest;
   
   public class SimpleTest {
   
    @ParameterizedTest
    @ValueSource(strings = { "one", "two", "three" })
    public void testIt(String name) throws Exception {
    System.err.println(name);
    }
   
    @RepeatedTest(value = 3)
    public void testRep() {
    System.err.println("hello");
    }
   
    @RepeatedTest(value = 3)
    public void testRep2() {
    System.err.println("hello2");
    }
   }
   ```
   Currently in Ant 1.10.7 (which has that fix for bz-63680), where the legacy reporting name gets used, the generated XML report is of the form:
   ```
   <testcase classname="org.myapp.SimpleTest" name="testIt(String)[3]" time="0.001"></testcase><testcase classname="org.myapp.SimpleTest" name="testIt(String)[2]" time="0.0"></testcase><testcase classname="org.myapp.SimpleTest" name="testRep2()[2]" time="0.001"></testcase><testcase classname="org.myapp.SimpleTest" name="testRep2()[1]" time="0.0"></testcase><testcase classname="org.myapp.SimpleTest" name="testRep()[2]" time="0.001"></testcase><testcase classname="org.myapp.SimpleTest" name="testRep()[1]" time="0.001"></testcase><testcase classname="org.myapp.SimpleTest" name="testRep()[3]" time="0.0"></testcase><testcase classname="org.myapp.SimpleTest" name="testRep2()[3]" time="0.0"></testcase><testcase classname="org.myapp.SimpleTest" name="testIt(String)[1]" time="0.028"></testcase>
   ```
   and the HTML looks like this image https://home.apache.org/~jaikiran/ant-1.10.7.png
   
   Now with the proposed change in this PR, the generated XML report looks like:
   ```
   <testcase classname="org.myapp.SimpleTest" name="[3] three" time="0.001"></testcase><testcase classname="org.myapp.SimpleTest" name="[2] two" time="0.0"></testcase><testcase classname="org.myapp.SimpleTest" name="repetition 2 of 3" time="0.001"></testcase><testcase classname="org.myapp.SimpleTest" name="repetition 1 of 3" time="0.0"></testcase><testcase classname="org.myapp.SimpleTest" name="repetition 2 of 3" time="0.001"></testcase><testcase classname="org.myapp.SimpleTest" name="repetition 1 of 3" time="0.0"></testcase><testcase classname="org.myapp.SimpleTest" name="repetition 3 of 3" time="0.001"></testcase><testcase classname="org.myapp.SimpleTest" name="repetition 3 of 3" time="0.0"></testcase><testcase classname="org.myapp.SimpleTest" name="[1] one" time="0.021"></testcase>
   ```
   and the report HTML looks like https://home.apache.org/~jaikiran/proposed-change.png
   
   Notice that the names no longer contain the method names and have completely lost context of which method is being repeat tested or parameterized.
   The version in 1.10.7 has the necessary information about the methods being parameterized as well as repeated, so I believe we should continue to use the way it is in 1.10.7.
   
   
   
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [ant] mguillem commented on issue #121: use displayName instead of legacyReportingName in xml reports

GitBox
In reply to this post by GitBox
mguillem commented on issue #121: use displayName instead of legacyReportingName in xml reports
URL: https://github.com/apache/ant/pull/121#issuecomment-585109817
 
 
   Hi @jaikiran,
   I understand the problems with the usage of `displayName`.
   
   What do you think of these first ideas to solve this issue without the problems you mention:
   - use `${legacyReportingName}: ${displayName}` as test name, when `legacyReportingName` and `displayName` differ (I haven't yet verified when it is the case)
   - alternative: add an option to the JUnitLauncher task to specifiy which name should be used

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [ant] mguillem edited a comment on issue #121: use displayName instead of legacyReportingName in xml reports

GitBox
In reply to this post by GitBox
mguillem edited a comment on issue #121: use displayName instead of legacyReportingName in xml reports
URL: https://github.com/apache/ant/pull/121#issuecomment-585109817
 
 
   Hi @jaikiran,
   I understand the problems with the usage of `displayName`.
   
   What do you think of these first ideas to solve this issue without the problems you mention:
   - use something like `${legacyReportingName}: ${displayName}` as test name, when `legacyReportingName` and `displayName` differ (I haven't yet verified when it is the case)
   - alternative: add an option to the JUnitLauncher task to specifiy which name should be used

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [ant] jaikiran commented on issue #121: use displayName instead of legacyReportingName in xml reports

GitBox
In reply to this post by GitBox
jaikiran commented on issue #121: use displayName instead of legacyReportingName in xml reports
URL: https://github.com/apache/ant/pull/121#issuecomment-585148348
 
 
   > alternative: add an option to the JUnitLauncher task to specifiy which name should be used
   
   Yes, I had this in mind but hadn't seen a necessity yet. But I think it makes sense to let users decide what names they want reported (not just for this formatter, but other ones as well). Can you file a bugzilla enhancement request for this?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]