Moved creation of per test data to testStarted method.

Review Request #2257 - Created May 22, 2015 and submitted

Information
Fedor Korotkov
pants
1234, 1588
ae81866...
Reviewers
pants-reviews
jsirois, stuhood

During an upgrade of Pants at Twitter I found out that some of our tests are using org.powermock.modules.junit4.PowerMockRunner and it doesn't call fireTestRunStarted at all. We were getting NPEs because of that.

According to the docs testRunStarted and testStarted have pretty much the same meaning except testStarted is invoked just before an execution in the same thread.

Build a jar and used it with out tests and they passed.

Eric Ayers
Fedor Korotkov
Fedor Korotkov
Fedor Korotkov
Stu Hood
John Sirois
Fedor Korotkov
Review request changed

Status: Closed (submitted)

Tejal Desai

The power PowerMockRunNotifier.java in 1.16.2 does use
https://github.com/jayway/powermock/blame/9bd63da355f0ab862880e72976770ae44851f6c7/modules/module-impl/junit4/src/main/java/org/powermock/modules/junit4/internal/impl/PowerMockRunNotifier.java#L96

This rb is causing a regression where, the number of tests returned using registerTests() is always 1.

  1. I was debugging it and it never invoked testRunStarted(my breakpoint was never hitted).

    Could you please explain more? What happens when there is only one test?

  2. Sure: while debugging a TestCase where there are 2 test methods, i found out that only output from one test would get written in the <test>.out.txt.

    This is because, registerTests() would return the count of Tests as 1.
    In testStarted gets invoked with testmethod (hence the number of tests is 1) instead of testclass.
    This will set the useCount=1 and when testFinished is fired for the first test, it would close the file stream.

    https://github.com/pantsbuild/pants/blob/master/src/java/org/pantsbuild/tools/junit/ConsoleRunner.java#L134 and
    https://github.com/pantsbuild/pants/blob/master/src/java/org/pantsbuild/tools/junit/ConsoleRunner.java#L217

    I am more inclined on reverting this and upgrading to powermock-1.6 if you see the null pointer expecption.

    On the other hand:
    Do you remember the target which failed inside twitter?
    I tried few targets in science which depend on powermock and they all seem to pass.

  3. There are number of cases where testRunStarted or testStarted is never invoked. See https://rbcommons.com/s/twitter/r/2385/. I'm working on the refactoring John spoke about in that PR at the moment.

  4. It seems it's easy to reproduce in tests:

    diff --git a/testprojects/tests/java/org/pantsbuild/testproject/dummies/PassingTest.java b/testprojects/tests/java/org/pantsbuild/testproject/dummies/PassingTest.java
    index 31173ce..3c86b63 100644
    --- a/testprojects/tests/java/org/pantsbuild/testproject/dummies/PassingTest.java
    +++ b/testprojects/tests/java/org/pantsbuild/testproject/dummies/PassingTest.java
    @@ -4,8 +4,14 @@ import org.junit.Test;
    
     public class PassingTest {
       @Test
    -  public void testPass() {
    +  public void testPass1() {
         // used in JunitTestsIntegrationTest#test_junit_test_suppress_output_flag
    -    System.out.println("Hello from test!");
    +    System.out.println("Hello from test1!");
    +  }
    +
    +  @Test
    +  public void testPass2() {
    +    // used in JunitTestsIntegrationTest#test_junit_test_suppress_output_flag
    +    System.out.println("Hello from test2!");
       }
     }
    diff --git a/tests/python/pants_test/tasks/test_junit_tests_integration.py b/tests/python/pants_test/tasks/test_junit_tests_integration.py
    index 0ed697b..84b2895 100644
    --- a/tests/python/pants_test/tasks/test_junit_tests_integration.py
    +++ b/tests/python/pants_test/tasks/test_junit_tests_integration.py
    @@ -188,4 +188,5 @@ class JunitTestsIntegrationTest(PantsRunIntegrationTest):
             'test.junit',
             '--no-suppress-output',
             'testprojects/tests/java/org/pantsbuild/testproject/dummies:passing_target'])
    -    self.assertTrue('Hello from test!' in pants_run.stdout_data)
    +    self.assertTrue('Hello from test1!' in pants_run.stdout_data)
    +    self.assertTrue('Hello from test2!' in pants_run.stdout_data)
    

    Could one of you who are working on a PR include the diff above to avoid regressions in future. Currently it's failing on master.

  5. Oh. It seems Collections.singletonList(description) should be changed to description.getChildren().

    So the counter will be correct and all streams will be closed and flushed. Seems currently thay are remain open even after all tests are finished.

  6. its the reverse. Streams are closed before all tests run.
    Also, as i mentioned earlier, testStarted get invoked with the "test Method" as the description. description.getChildren() on test method will be 0.

Loading...