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

          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.
          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)
          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.
          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.
          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.
          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.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development