Skip to content
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

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

Merged
merged 2 commits into from Aug 11, 2017

Conversation

Projects
None yet
6 participants
@kgyrtkirk
Copy link
Contributor

commented Aug 22, 2016

JENKINS-37598

  • 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 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

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

This comment has been minimized.

Copy link
@jglick

jglick Aug 29, 2016

Member

Seems like the simpler patch would just be

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

right?

This comment has been minimized.

Copy link
@jglick

jglick Aug 29, 2016

Member

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.

This comment has been minimized.

Copy link
@kgyrtkirk

kgyrtkirk Aug 29, 2016

Author Contributor

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

* @param sr
*/
public void merge(SuiteResult sr) {
if (hasTimeAttr) {

This comment has been minimized.

Copy link
@jglick

jglick Aug 29, 2016

Member

Should that be sr.hasTimeAttr?

This comment has been minimized.

Copy link
@jglick

jglick Aug 29, 2016

Member

Would be resolved by test cases checking

<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"/>
    </testsuite>
    <testsuite name="test.fs.FileSystemTests" time="50">
      <testcase name="testPrefix2" classname="test.fs.FileSystemTest1" time="10"/>
    </testsuite>
</testsuites>

and

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

both of which I would expect to report a total time of 60. (Right?)

This comment has been minimized.

Copy link
@kgyrtkirk

kgyrtkirk Aug 29, 2016

Author Contributor

...i think we will have time attributest on all testuites or neither of them...i didn't wanted to get lost in the mixed case...but if you think it's important i can fix that ;)

This comment has been minimized.

Copy link
@kgyrtkirk

kgyrtkirk Aug 29, 2016

Author Contributor

yes that should be hasTimeAttr .. this is the closest currently which rely on the previously mentionet - all of them has it or neither - logic....

@kgyrtkirk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

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

This comment has been minimized.

Copy link
Member

commented Sep 24, 2016

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 kgyrtkirk force-pushed the kgyrtkirk:JENKINS-37598 branch from c27b280 to 0ddde97 Sep 26, 2016

@kgyrtkirk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2016

@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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2016

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

@kgyrtkirk

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2017

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

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 16, 2017

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

JENKINS-37598: Inaccurate aggregation of multiple xmls containing cas…
…e information about the same testsuite

@kgyrtkirk kgyrtkirk force-pushed the kgyrtkirk:JENKINS-37598 branch from 0ddde97 to 8f3473b Feb 15, 2017

@kgyrtkirk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2017

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

@@ -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;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 16, 2017

Member

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

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Feb 16, 2017

Member

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

This comment has been minimized.

Copy link
@kgyrtkirk

kgyrtkirk Mar 13, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 16, 2017

Member

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

*/
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()+")");

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 16, 2017

Member

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

This comment has been minimized.

Copy link
@kgyrtkirk

kgyrtkirk Mar 13, 2017

Author Contributor

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.

@kgyrtkirk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2017

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

@kgyrtkirk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2017

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

@oleg-nenashev
Copy link
Member

left a comment

Looks good to me. Sorry for missing the previous ping

@kgyrtkirk

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2017

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)

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

1 check passed

Jenkins This pull request looks good
Details

mfuchs added a commit to mfuchs/junit-plugin that referenced this pull request Aug 18, 2017

JENKINS-42438 Correctly calculate test class duration.
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.

@bparees bparees referenced this pull request Aug 23, 2017

Merged

Add crt file env var #152

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

stop calling junit merge.
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 30, 2017

stop calling junit merge.
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.
since jenkinsci/junit-plugin#54 was merged,
it should not be needed anymore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.