You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@maven.apache.org by lamyaa <gi...@git.apache.org> on 2015/06/16 00:12:29 UTC
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
GitHub user lamyaa opened a pull request:
https://github.com/apache/maven-surefire/pull/98
[Surefire-1144] Time for testsuite on commandline does not suit with the time value given in the report file
https://issues.apache.org/jira/browse/SUREFIRE-1144
This pull request modifies the XML reporter such that it shows time computed as `endTime - startTime` rather than summing up the method execution times through `getRunTimeForAllTests()`.
I have tested this patch with the project provided by Karl in the bug report thread:
https://github.com/khmarbaise/supose/
The time shown in the XML report is now consistent with the one shown in the console.
@Tibor17 I would appreciate your input on a couple of things:
1) Initially, I deleted line 111/122 (`elapsedForTestSet += elapsedForThis;`) in TestSetStats and line 100 (` elapsedForTestSet = testSetEndAt - testSetStartAt;)` was not within an if-block. However, that made XmlReporterRunTimeIT fail. The IT runs test methods in parallel and `testSetEndAt - testSetStartAt` ends up being abnormally small, smaller than the sleep times of any of the test methods. I don't understand enough of how the parallelism and fork code is implemented to properly debug that so I left line 111/122 and wrapped line 100 within an if-block. Please let me know if you have a better idea of how to deal with this.
2) The XML report now prints `elapsedForTestSet = testSetEndAt - testSetStartAt` but the console (which I have not changed) prints `elapsedSinceTestSetStart = currentTime - testSetStartAt`. This means the console time may be 1 or 2ms more than the XML report time. Do you think it would be good to have the console also print `elapsedForTestSet`? If not, I probably need to change Surefire1144XmlRunTimeIT to accept a 1 or 2ms difference.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lamyaa/maven-surefire surefire-1144
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/maven-surefire/pull/98.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #98
----
commit 9d4931b9ccd82e9305efe1405a56083b6023e84e
Author: lamyaa <el...@illinois.edu>
Date: 2015-06-12T19:29:58Z
[Surefire-1144] Time for testsuite on commandline does not suit with the time value given in the report file
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-143595094
@lamyaa
Can I finish it unless you you are already in progress?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-143544670
@lamyaa
I applied the local change and executed XmlReporterRunTimeIT.
The balancer is really applied and Test1 was really executed within 600 millis.
Test2 was also balanced and finished after 500 millis- that's ok, but I do not understand two things:
+ why the loop in XmlReporterRunTimeIT iterates over one report, means Test1
+ how the balancer initiated running the tests twice (first dummy run to create dummy file `.surefire-<hash>`, and second normal run)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-139858232
@lamyaa
I think I found a solution which works on my side with non-parallel tests.
Feel free to tell me what you think about my hypothesis.
Can you please try to create a new pull request and call `testSetReportEntry.elapsedTimeAsString(); break;` after the switch-case line `case success:` in class `StatelessXmlReporter`?
I am not sure what we should do with elapsed time in case error and case failure; maybe the same but this could be also answered by a test.
Your IT is cool, but we can extend it with profiles: testng, junit4, junit47 and parallel or non-parallel. All of these produce different exec time.
As a hint on how to extent the IT with profiles, please see this classes `AbstractTestMultipleMethodPatterns` or `AbstractFailFastIT` and the subclass. It helps on how to write parameterized test with profiles in IT. I guess you would need one IT for JUnit and another for TestNG.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by lamyaa <gi...@git.apache.org>.
Github user lamyaa commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-140152959
@Tibor17
``createTestSuiteElement( ppw, testSetReportEntry, testSetStats, reportNameSuffix, testSetReportEntry.elapsedTimeAsString() )`` does fix the issue.
Shall I make a new pull request with this change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by lamyaa <gi...@git.apache.org>.
Github user lamyaa closed the pull request at:
https://github.com/apache/maven-surefire/pull/98
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by lamyaa <gi...@git.apache.org>.
Github user lamyaa commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-143607529
Thanks for taking care of this @Tibor17
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-141173488
@lamyaa
I will have a look into this test.
On Thu, Sep 17, 2015 at 7:19 PM, lamyaa <no...@github.com> wrote:
> @Tibor17 <https://github.com/Tibor17> As I was preparing a new pull
> request with the simple change:
> createTestSuiteElement( ppw, testSetReportEntry, testSetStats,
> reportNameSuffix, testSetReportEntry.elapsedTimeAsString() )
> I noticed that it makes XmlReporterRunTimeIT fail. Specifically, this
> assertion:
> assertTrue( "runorder.parallel.Test1 report.getTimeElapsed found:" +
> report.getTimeElapsed(),
> report.getTimeElapsed() >= 1.2f );
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/maven-surefire/pull/98#issuecomment-141156540>.
>
--
Cheers
Tibor
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by lamyaa <gi...@git.apache.org>.
Github user lamyaa commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-140148135
@Tibor17 I tried just changing the last parameter like you suggested:
``createTestSuiteElement( ppw, testSetReportEntry, testSetStats, reportNameSuffix, testSetReportEntry.elapsedTimeAsString() )``
However, that does not fix the test case in https://github.com/khmarbaise/supose/.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-143606448
@lamyaa
Pls close this pull-request. I have pushed the fix including your IT.
Thx for the IT, it was very nice work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-139915061
IMHO all we need to do is to change the last parameter:
`createTestSuiteElement( ppw, testSetReportEntry, testSetStats, reportNameSuffix, testSetReportEntry.elapsedTimeAsString() )`
I think we should check if `elapsedTimeAsString()` can throw NPE doe to the elapsed time is `Integer`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by lamyaa <gi...@git.apache.org>.
Github user lamyaa commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-141156540
@Tibor17 As I was preparing a new pull request with the simple change:
``createTestSuiteElement( ppw, testSetReportEntry, testSetStats, reportNameSuffix, testSetReportEntry.elapsedTimeAsString() )``
I noticed that it makes ``XmlReporterRunTimeIT`` fail. Specifically, this assertion:
`` assertTrue( "runorder.parallel.Test1 report.getTimeElapsed found:" + report.getTimeElapsed(),
report.getTimeElapsed() >= 1.2f );``
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by lamyaa <gi...@git.apache.org>.
Github user lamyaa commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-143595744
@Tibor17 I'm very sorry for the delay. Please go ahead and finish this one if you have time.
I'm not quite sure how XmlReporterRunTimeIT works either. I noticed the same on my machine, Test1 finishes in a bit over 600ms and Test2 finishes in a bit over 500ms.
Not sure why it iterates over one report, though, or how the balancer initiates running the tests twice...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-140308340
Yes a new pull request would be great. I have made a lot of rework in
surefire. Most probably this branch is not able to merge.
On 9/14/15, lamyaa <no...@github.com> wrote:
> @Tibor17
> ``createTestSuiteElement( ppw, testSetReportEntry, testSetStats,
> reportNameSuffix, testSetReportEntry.elapsedTimeAsString() )`` does fix the
> issue.
> Shall I make a new pull request with this change?
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/apache/maven-surefire/pull/98#issuecomment-140152959
--
Cheers
Tibor
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by lamyaa <gi...@git.apache.org>.
Github user lamyaa commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-140153614
@Tibor17 It seems that we have a check for elapsed time being null in TestSetRunListerner.java so we never instantiate WrappedReportEntry with a null elapsed time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-138134180
@lamyaa
Sorry for waiting so long time. I have finished issue 580.
I will have a look on this since now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request: [Surefire-1144] Time for testsuite on...
Posted by lamyaa <gi...@git.apache.org>.
Github user lamyaa commented on the pull request:
https://github.com/apache/maven-surefire/pull/98#issuecomment-139917934
@Tibor17 Please give me a couple of days to look again at this. It's not very fresh in my mind anymore :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org