JENKINS-42438 Correctly calculate test class duration.

[mfuchs]

When running Android tests the in the generated xml file
can contain multiple classes.
Thus setting the test class duration to the value of the test-suite is wrong.

[ryncsn]

This reverts some changes by #35 , maybe it's better to set the duration to re-sum of test cases only when there are multiple classes in a test suite? or add a configure option?

[mfuchs]

Thanks for your input! :)

In my opinion #35 introduced a bug and the person reporting JENKINS-42438 apparently has the same issue.
So I don't think I want to keep that specific behaviour of #35, especially as no reason other than "it's in the spec" was given in #35, while no spec was linked.
Also a spec is only as useful as their implementations.

I am afraid of adding even more complexity, like deciding whether a test suite has multiple classes and dynamically having different behaviours in ClassResult.tally() as a result.
Especially when I can't see the benefit.
It would make this PR even harder to get in.

Of course, if those changes of #35 also fixed a bug themselves when that would be different.

@jmarnold1021 could you please chime in on the class duration changes you introduced with #35?

[oleg-nenashev]

I am not convinced this is a right fix. It just reverts the breaking change (though it could be a proper hotfix). IMHO it would be better to have a proper parsing of floats instead

[mfuchs]

In what way is the issue related to parsing floats so that I could look into that?

Btw. do you have a link to the JUnit XML specification? I am probably just too darn blind to find it.

[oleg-nenashev]

Here is a good thread about processing thousand separators via DecimalFormat in Java: https://stackoverflow.com/questions/4323599/best-way-to-parsedouble-with-comma-as-decimal-separator

The discussion is not so motivating though. The format depends on the locale of the input file, and I am not sure how to extract it reliably from XML.

[mfuchs]

I am not sure how this PR is related to parsing floats.
The only difference is what floats are summed: In my case I sum the time (floats) of the testcases belonging to a class, while the existing code sums the time (floats) of the whole testsuites.

The existing code assumes that one testsuite always represents one class.
This is not the case when running tests on android devices, e.g. the android emulator.

[abayer]

@mfuchs I wonder if it might make sense to use the SuiteResult durations if and only if every CaseResult in those SuiteResults is in this ClassResult, and otherwise use the CaseResult durations?

[mfuchs]

@oleg-nenashev can you please re-review this PR? I don't think that your review comment applies here as I never parse floats.

[mfuchs]

@abayer I am not sure what the benefit of the added complexity would be.

[mfuchs]

@olivergondza Can you please also review this PR?

[olivergondza]

The way I understand it is the ClassResult and SuiteResult are orthogonal views of a set of CaseResults - one grouped per declaration class, the other per report file. Hence it appears correct to me to iterate over the nested CaseResults in both cases instead of depending on the fact that specific relationship exists between ClassResult and SuiteResult.

And this appears what the fix is trying to achieve.

---

@abayer, I do not see why that would not work but it is more elaborate compared to ignoring SuiteResult for computing ClassResult's duration time entirely. Does it provide more accurate results or why would we want to do that?

---

@oleg-nenashev, please elaborate on how float parsing is related. We suspect that it is not...

I have no memory of this place

[olivergondza]

@abayer, any comments? I am tempted to merge this...

[abayer]

Fine by me!