Hadoop Common
  1. Hadoop Common
  2. HADOOP-7537

Add PowerMock for the development of better tests

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: build
    • Labels:
      None

      Description

      We already have Mockito, but PowerMock extends its capabilties so that we can mock constructors and static methods. I find that it is extremely difficult, if not impossible, to properly test some of the low-level features without this.

        Issue Links

          Activity

          Kihwal Lee created issue -
          Kihwal Lee made changes -
          Field Original Value New Value
          Link This issue is blocked by HADOOP-7536 [ HADOOP-7536 ]
          Hide
          Kihwal Lee added a comment -

          This Jira depends on HADOOP-7536, which will restore the version of commons-logging to 1.1.1.

          Show
          Kihwal Lee added a comment - This Jira depends on HADOOP-7536 , which will restore the version of commons-logging to 1.1.1.
          Kihwal Lee made changes -
          Link This issue blocks HADOOP-7492 [ HADOOP-7492 ]
          Kihwal Lee made changes -
          Attachment powermock.patch [ 12490165 ]
          Hide
          Jeffrey Naisbitt added a comment -

          Could we add this to http://wiki.apache.org/hadoop/HowToDevelopUnitTests (where it briefly describes using mockito, we could mention powermock)

          Show
          Jeffrey Naisbitt added a comment - Could we add this to http://wiki.apache.org/hadoop/HowToDevelopUnitTests (where it briefly describes using mockito, we could mention powermock)
          Kihwal Lee made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12490165/powermock.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/45//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/45//artifact/trunk/target/newPatchFindbugsWarningshadoop-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/45//artifact/trunk/target/newPatchFindbugsWarningshadoop-annotations.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/45//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12490165/powermock.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/45//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/45//artifact/trunk/target/newPatchFindbugsWarningshadoop-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/45//artifact/trunk/target/newPatchFindbugsWarningshadoop-annotations.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/45//console This message is automatically generated.
          Hide
          Kihwal Lee added a comment -

          No test needed as the patch only adds dependencies.

          Show
          Kihwal Lee added a comment - No test needed as the patch only adds dependencies.
          Kihwal Lee made changes -
          Description We already have Mockito, but PowerMock extends its capabilties so that we can mock constructors and static methods. I find that it is extremely difficult, if not impossible, to properly test some of the low-lever features without this. We already have Mockito, but PowerMock extends its capabilties so that we can mock constructors and static methods. I find that it is extremely difficult, if not impossible, to properly test some of the low-level features without this.
          Hide
          Eric Yang added a comment -

          Dependency for Hadoop project doesn't specify the scope. Should this be for testing only for Hadoop-project?

          Show
          Eric Yang added a comment - Dependency for Hadoop project doesn't specify the scope. Should this be for testing only for Hadoop-project?
          Hide
          Todd Lipcon added a comment -

          I've also often wanted PowerMock. My only question is whether we should treat its usage as a red flag – I think often it's indicatve of a design issue in the code. Where did you run into something that was untestable without it?

          Show
          Todd Lipcon added a comment - I've also often wanted PowerMock. My only question is whether we should treat its usage as a red flag – I think often it's indicatve of a design issue in the code. Where did you run into something that was untestable without it?
          Hide
          Kihwal Lee added a comment -

          @eric I think no dependency in hadoop-project has its scope defined. The pom.xml in each subproject contains scope.

          @todd I am using it for the RPC server address change detection test. It requires mocking the constructor of InetSocketAddress.

          Show
          Kihwal Lee added a comment - @eric I think no dependency in hadoop-project has its scope defined. The pom.xml in each subproject contains scope. @todd I am using it for the RPC server address change detection test. It requires mocking the constructor of InetSocketAddress.
          Hide
          Scott Carey added a comment -

          I have had mostly bad experiences with PowerMock. As Todd says, it is usually indicative of something that needs to be refactored or fixed; or is simply mis-used.

          Tests that use too much PowerMock are extremely brittle, often failing in test code B after after changes in seemingly unrelated code A in ways that require deep knowledge of B. It does some funky stuff in classloader land that results in painful to debug issues. For example, I have seen static code initialized twice, leading to a test that hangs forever.

          We have started to use phrases like "friends don't let friends abuse PowerMock". Fixing a broken PowerMock test takes much longer than fixing an ordinary test.

          It is a useful tool to have around, but needs to be used judiciously. Every time it is used "because I must mock XYZ" ask, "why does XYZ need to be mocked? does it need changes to design to be testable?"

          Show
          Scott Carey added a comment - I have had mostly bad experiences with PowerMock. As Todd says, it is usually indicative of something that needs to be refactored or fixed; or is simply mis-used. Tests that use too much PowerMock are extremely brittle, often failing in test code B after after changes in seemingly unrelated code A in ways that require deep knowledge of B. It does some funky stuff in classloader land that results in painful to debug issues. For example, I have seen static code initialized twice, leading to a test that hangs forever. We have started to use phrases like "friends don't let friends abuse PowerMock". Fixing a broken PowerMock test takes much longer than fixing an ordinary test. It is a useful tool to have around, but needs to be used judiciously. Every time it is used "because I must mock XYZ" ask, "why does XYZ need to be mocked? does it need changes to design to be testable?"
          Hide
          Kihwal Lee added a comment -

          I understand the concerns. If we decide to add PowerMock, a strong warning should be put at http://wiki.apache.org/hadoop/HowToDevelopUnitTests.

          The test I am writing is for detecting state changes external to JVM. E.g. IP address changes.

          Show
          Kihwal Lee added a comment - I understand the concerns. If we decide to add PowerMock, a strong warning should be put at http://wiki.apache.org/hadoop/HowToDevelopUnitTests . The test I am writing is for detecting state changes external to JVM. E.g. IP address changes.
          Hide
          Todd Lipcon added a comment -

          The way we've mocked things like this in HBase is to use a class called "EnvironmentEdge". The default implementation simply forwards calls to the normal JVM implementations (eg envEdge.currentTimeMillis() calls System.currentTimeMillis()). It's then easy to mock this by replacing an instance within a particular class with a mock. Could we do the same thing with the construction of InetSocketAddress?

          Show
          Todd Lipcon added a comment - The way we've mocked things like this in HBase is to use a class called "EnvironmentEdge". The default implementation simply forwards calls to the normal JVM implementations (eg envEdge.currentTimeMillis() calls System.currentTimeMillis()). It's then easy to mock this by replacing an instance within a particular class with a mock. Could we do the same thing with the construction of InetSocketAddress?
          Hide
          Kihwal Lee added a comment -

          I took a look at EnvironmentEdge in HBase.
          Maybe it's an ignorant question, but can this be a security hole?

          Show
          Kihwal Lee added a comment - I took a look at EnvironmentEdge in HBase. Maybe it's an ignorant question, but can this be a security hole?
          Hide
          Todd Lipcon added a comment -

          How so? If you have access to arbitrarily put new untrusted classes on the classpath of a server, then we've already lost...

          Show
          Todd Lipcon added a comment - How so? If you have access to arbitrarily put new untrusted classes on the classpath of a server, then we've already lost...
          Hide
          Kihwal Lee added a comment -

          We should still discuss alternative ways of design/testing, but I am looking for a brave soul who can put the last nail on the coffin. If someone -1 the idea/patch, I will close it as "won't fix". Scott? Todd?

          Show
          Kihwal Lee added a comment - We should still discuss alternative ways of design/testing, but I am looking for a brave soul who can put the last nail on the coffin. If someone -1 the idea/patch, I will close it as "won't fix". Scott? Todd?
          Hide
          Todd Lipcon added a comment -

          I think EnvironmentEdge is a better approach, and would prefer to see this used as sparingly as possible. But I don't feel so strongly as to -1 it. I may -1 other patches in the future that use powermock, though, when a better design would be possible. Of course, I'm also not going to +1 it

          Show
          Todd Lipcon added a comment - I think EnvironmentEdge is a better approach, and would prefer to see this used as sparingly as possible. But I don't feel so strongly as to -1 it. I may -1 other patches in the future that use powermock, though, when a better design would be possible. Of course, I'm also not going to +1 it
          Hide
          Luke Lu added a comment -

          It requires mocking the constructor of InetSocketAddress.

          Instead of introducing PowerMock, which can lead to test failures in unrelated tests. How about just introduce an InetSocketAddress factory? In this particular case, what you really want to control is the DNS resolution. How about just introduce a NodeNameResolver interface and a DefaultNodeNameResolver singleton to manage the resolver (so you can use a test resolver to supply mocks)?

          If we find that we need to do too many such exercises, we'd better off using a complete (but still lightweight) DI solution like Guice (which can be introduced into our code incrementally), which is already a maven dependency in our code base

          Show
          Luke Lu added a comment - It requires mocking the constructor of InetSocketAddress. Instead of introducing PowerMock, which can lead to test failures in unrelated tests. How about just introduce an InetSocketAddress factory? In this particular case, what you really want to control is the DNS resolution. How about just introduce a NodeNameResolver interface and a DefaultNodeNameResolver singleton to manage the resolver (so you can use a test resolver to supply mocks)? If we find that we need to do too many such exercises, we'd better off using a complete (but still lightweight) DI solution like Guice (which can be introduced into our code incrementally), which is already a maven dependency in our code base
          Hide
          Kihwal Lee added a comment -

          Thank you all for suggestions. I will close this bug as "won't fix' and look for other ways.

          Show
          Kihwal Lee added a comment - Thank you all for suggestions. I will close this bug as "won't fix' and look for other ways.
          Kihwal Lee made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Kihwal Lee made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Kihwal Lee made changes -
          Status In Progress [ 3 ] Open [ 1 ]
          Kihwal Lee made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Won't Fix [ 2 ]
          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          Radim Kolar added a comment -

          Test fragility is related to using mock method of testing. In ideal world, mock is used for integration tests.

          Show
          Radim Kolar added a comment - Test fragility is related to using mock method of testing. In ideal world, mock is used for integration tests.
          Colin Patrick McCabe made changes -
          Link This issue is duplicated by HADOOP-9122 [ HADOOP-9122 ]
          Hide
          Radim Kolar added a comment -

          Powermock does not introduce considerable new dangers compared to already used mockito solution. I do not have experience that powermock break something unless it is related to class loading and then you have to setup powermock properly to ignore loaded on demand classes.

          Show
          Radim Kolar added a comment - Powermock does not introduce considerable new dangers compared to already used mockito solution. I do not have experience that powermock break something unless it is related to class loading and then you have to setup powermock properly to ignore loaded on demand classes.
          Gavin made changes -
          Link This issue blocks HADOOP-7492 [ HADOOP-7492 ]
          Gavin made changes -
          Link This issue is depended upon by HADOOP-7492 [ HADOOP-7492 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          5d 20h 26m 1 Kihwal Lee 17/Aug/11 15:54
          Patch Available Patch Available Open Open
          20d 37m 1 Kihwal Lee 06/Sep/11 16:32
          Open Open In Progress In Progress
          36s 1 Kihwal Lee 06/Sep/11 16:33
          In Progress In Progress Open Open
          11s 1 Kihwal Lee 06/Sep/11 16:33
          Open Open Resolved Resolved
          23s 1 Kihwal Lee 06/Sep/11 16:33
          Resolved Resolved Closed Closed
          69d 9h 17m 1 Arun C Murthy 15/Nov/11 00:50

            People

            • Assignee:
              Kihwal Lee
              Reporter:
              Kihwal Lee
            • Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development