[JENKINS-37598] Inaccurate aggregation of multiple xmls containing case information about the same testsuite #54

kgyrtkirk · 2016-08-22T21:29:22Z

change logic to account for all suites involved during time aggregation in ClassResult
moved TestSuite merging code from TestResult to TestSuite.merge()
replaced TestSuite.time field with a boolean
added 2 testcases - 1 with time attributes; and 1 without them...added to TestResultTest (because of TestSuite.merge() call location)

jglick · 2016-08-29T16:16:37Z

src/main/java/hudson/tasks/junit/ClassResult.java

+        // retrieve the class duration from these cases' suite time
+        duration = 0;
+        for (SuiteResult s : suites) {
+            duration += s.getDuration();


Seems like the simpler patch would just be

-if(duration == 0){ - duration = r.getSuiteResult().getDuration(); -} +duration += r.getSuiteResult().getDuration();

right?

BTW neither of the test cases seem to check that

<testsuites name="Automation Tests" tests="2" errors="0" failures="0" ignored="0"> <testsuite name="test.fs.FileSystemTests"> <testcase name="testPrefix1" classname="test.fs.FileSystemTest1" time="10"/> <testcase name="testPrefix1" classname="test.fs.FileSystemTest2" time="10"/> </testsuite> <testsuite name="test.fs.FileSystemTests"> <testcase name="testPrefix2" classname="test.fs.FileSystemTest1" time="10"/> </testsuite> </testsuites>

sums to 30.

i'm afraid not...and a change like that would be caught by the one of my new tests
in case not...i should add a case for this too ;)

..the for cycle is running thru cases...and the old trick was to only use the case runtime when the total is 0...the new code collects all the unique suites and aggregates there sum time

kgyrtkirk · 2016-09-04T21:16:54Z

I've updated the testcases - should I squash my changes - instead of adding more commits?

About the mixed cases - i can detect it...but not sure what to do when that happens...throw an exception; or insert a "notice testcase" into the unit results?

    public void merge(SuiteResult sr) {
      if (sr.hasTimeAttr ^ hasTimeAttr){
          // addCase(new CaseResult(this, "__jenkins-junit-plugin_invalid-suite-merge", ""));
          // or throw new RuntimeException()
      }
      ...

oleg-nenashev · 2016-09-24T09:26:31Z

I've updated the testcases - should I squash my changes - instead of adding more commits?

No need to do it. GitHub allows squashing during the merge

About the mixed cases - i can detect it...but not sure what to do when that happens...throw an exception; or insert a "notice testcase" into the unit results?

Exception is unlikely a good approach. I would merge with warning

kgyrtkirk · 2016-09-26T20:08:55Z

@oleg-nenashev i've updated the changes; and included an exception for the mixed case (plus a test case for this mixed case too)

cheers

kgyrtkirk · 2016-12-08T15:46:50Z

@oleg-nenashev: Can you please take a look at the updated changes?

kgyrtkirk · 2017-01-16T11:08:11Z

@oleg-nenashev can you please take a look at this change?

oleg-nenashev · 2017-01-16T11:13:50Z

@kgyrtkirk sorry, I was snowed under the stuff. Will try to review today.
Anyway, CC @jenkinsci/code-reviewers since more feedback is required

…e information about the same testsuite

kgyrtkirk · 2017-02-15T00:10:38Z

@jglick @oleg-nenashev - i've fixed the merge-conflict.

oleg-nenashev · 2017-02-16T07:08:51Z

src/main/java/hudson/tasks/junit/SuiteResult.java

@@ -72,8 +72,8 @@
    /** Optional ID attribute of a test suite. E.g., Eclipse plug-ins tests always have the name 'tests' but a different id. **/
    private String id;

-    /** Optional time attribute of a test suite. E.g., Suites can use their own time attribute or the sum of their cases' times as before.**/
-    private String time;


If the class is being persisted on the disk, it requires a migration logic in readResolve()

Removal of this field will probably result in old data warnings. Should be made deprecated and transient instead.

Oh...I wasn't aware of this; in this case I think the best would be to live with existing 'time' field; and use it

oleg-nenashev · 2017-02-16T07:10:12Z

src/main/java/hudson/tasks/junit/SuiteResult.java

+    /**
+     * Merges another SuiteResult into this one.
+     * 
+     * @param sr


If you define the parameter in Javadoc, please add its description. Otherwise Javadoc linters will grumble

oleg-nenashev · 2017-02-16T07:12:47Z

src/main/java/hudson/tasks/junit/SuiteResult.java

+     */
+    public void merge(SuiteResult sr) {
+        if (sr.hasTimeAttr ^ hasTimeAttr){
+            throw new IllegalStateException("Merging of suiteresults with incompatible time attribute usage is not supported.( "+getFile()+", "+sr.getFile()+")");


Since you introduce a new method, it would be preferable to propagate a non-RunTime exception and to make the code above handle it. If you want to stick to IllegalStateException , it should be documented at least

I've changed this exception into a logger call...since the severity of this is not really critical - and theres no proper way to handle this elsewhere...so it will log a warning in this case.

…throw exception

kgyrtkirk · 2017-03-24T08:11:30Z

@oleg-nenashev could you please take a look at this? I think the latest changes will look more promising :)

kgyrtkirk · 2017-04-21T17:20:28Z

@oleg-nenashev , @jglick, @jenkinsci/code-reviewers could you please take a look at this patch?

oleg-nenashev

Looks good to me. Sorry for missing the previous ping

kgyrtkirk · 2017-07-28T10:19:06Z

Could someone merge this? I would like to use this if possible from the next release (and drop the usage of a custom built plugin)

When running Android tests the <testsuite> in the generated xml file can contain multiple classes. Thus setting the test class duration to the value of the test-suite is wrong. Instead the class duration is now calculated as sum its test cases. This is the same way it was done before jenkinsci#35 was merged. Reverting parts of both jenkinsci#35 and jenkinsci#54 did not break any existing tests.

since jenkinsci/junit-plugin#54 was merged, it should not be needed anymore.

jglick changed the title ~~JENKINS-37598: Inaccurate aggregation of multiple xmls containing cas…~~ [JENKINS-37598] Inaccurate aggregation of multiple xmls containing case information about the same testsuite Aug 29, 2016

jglick reviewed Aug 29, 2016
View reviewed changes

kgyrtkirk force-pushed the JENKINS-37598 branch from c27b280 to 0ddde97 Compare September 26, 2016 19:57

smarterclayton mentioned this pull request Dec 28, 2016

Merge JUnit results from extended suites openshift/origin#12357

Merged

jglick added merge-conflict needs-review labels Feb 13, 2017

JENKINS-37598: Inaccurate aggregation of multiple xmls containing cas…

8f3473b

…e information about the same testsuite

kgyrtkirk force-pushed the JENKINS-37598 branch from 0ddde97 to 8f3473b Compare February 15, 2017 00:00

oleg-nenashev removed the merge-conflict label Feb 16, 2017

oleg-nenashev reviewed Feb 16, 2017

View reviewed changes

JENKINS-37598: address review comments; put back time field, doesn't …

319280f

…throw exception

oleg-nenashev assigned oleg-nenashev and unassigned oleg-nenashev Apr 21, 2017

oleg-nenashev approved these changes Apr 21, 2017

View reviewed changes

mc1arke approved these changes Apr 21, 2017

View reviewed changes

olivergondza merged commit d083744 into jenkinsci:master Aug 11, 2017

bparees mentioned this pull request Aug 23, 2017

Add crt file env var openshift/jenkins-plugin#152

Merged

bparees added a commit to bparees/origin that referenced this pull request Aug 30, 2017

stop calling junit merge.

2c35e23

since jenkinsci/junit-plugin#54 was merged, it should not be needed anymore.

bparees mentioned this pull request Aug 30, 2017

stop calling junit merge. openshift/origin#16068

Merged

bparees added a commit to bparees/origin that referenced this pull request Aug 30, 2017

stop calling junit merge.

8bc17ed

since jenkinsci/junit-plugin#54 was merged, it should not be needed anymore.

bparees added a commit to bparees/origin that referenced this pull request Aug 31, 2017

stop calling junit merge.

3f26fdf

since jenkinsci/junit-plugin#54 was merged, it should not be needed anymore.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-37598] Inaccurate aggregation of multiple xmls containing case information about the same testsuite #54

[JENKINS-37598] Inaccurate aggregation of multiple xmls containing case information about the same testsuite #54

kgyrtkirk commented Aug 22, 2016 •

edited

Loading

jglick Aug 29, 2016

jglick Aug 29, 2016

kgyrtkirk Aug 29, 2016

kgyrtkirk commented Sep 4, 2016

oleg-nenashev commented Sep 24, 2016

kgyrtkirk commented Sep 26, 2016

kgyrtkirk commented Dec 8, 2016

kgyrtkirk commented Jan 16, 2017

oleg-nenashev commented Jan 16, 2017

kgyrtkirk commented Feb 15, 2017

oleg-nenashev Feb 16, 2017

daniel-beck Feb 16, 2017

kgyrtkirk Mar 13, 2017

oleg-nenashev Feb 16, 2017

oleg-nenashev Feb 16, 2017

kgyrtkirk Mar 13, 2017

kgyrtkirk commented Mar 24, 2017

kgyrtkirk commented Apr 21, 2017

oleg-nenashev left a comment

kgyrtkirk commented Jul 28, 2017

[JENKINS-37598] Inaccurate aggregation of multiple xmls containing case information about the same testsuite #54

[JENKINS-37598] Inaccurate aggregation of multiple xmls containing case information about the same testsuite #54

Conversation

kgyrtkirk commented Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgyrtkirk commented Sep 4, 2016

oleg-nenashev commented Sep 24, 2016

kgyrtkirk commented Sep 26, 2016

kgyrtkirk commented Dec 8, 2016

kgyrtkirk commented Jan 16, 2017

oleg-nenashev commented Jan 16, 2017

kgyrtkirk commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgyrtkirk commented Mar 24, 2017

kgyrtkirk commented Apr 21, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

kgyrtkirk commented Jul 28, 2017

kgyrtkirk commented Aug 22, 2016 •

edited

Loading