Uploaded image for project: 'Accumulo'
  1. Accumulo
  2. ACCUMULO-3920

Deprecate mock and cease using it in our tests

    Details

    • Type: Task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8.0
    • Component/s: None
    • Labels:
      None

      Description

      Numerous prior discussions have occurred, expressing a desire to deprecate (deprecate is not the same as remove) mock, so we can properly communicate its current status as a "dying" (poorly maintained, not kept up-to-date, lacking features) component of Accumulo. Some interest has been expressed in keeping it around, but no work has been put in to it to fix it's numerous divergences from the behavior of "real" Accumulo, add its missing features, or to improve its API.

      The task here is to:

      • Cease and desist using mock in our tests (replacing it with MAC, EasyMock, etc., as appropriate)
      • Mark mock @Deprecated until it is eventually removed in a major release

      Further discussion, if necessary, can be held later regarding its actual removal.

        Issue Links

          Activity

          Hide
          ctubbsii Christopher Tubbs added a comment -

          Note: there are some places where Mock is still needed in our code, to support various functionality we've given first-class citizenship to for mock. This is primarily mock-related features in MapReduce and the shell. For these things, in order to stop using Mock and to avoid propagating up a bunch of deprecation warnings about its continued usage, I intend to create a non-public-API class called DeprecationUtil for use in the implementation which must remain. This class will provide static methods which wrap the existing calls directly to Mock, and will be responsible for suppressing deprecation warnings so that the non-Mock code which currently depends on Mock can remain in-tact as much as possible, until Mock is actually removed.

          Show
          ctubbsii Christopher Tubbs added a comment - Note: there are some places where Mock is still needed in our code, to support various functionality we've given first-class citizenship to for mock. This is primarily mock-related features in MapReduce and the shell. For these things, in order to stop using Mock and to avoid propagating up a bunch of deprecation warnings about its continued usage, I intend to create a non-public-API class called DeprecationUtil for use in the implementation which must remain. This class will provide static methods which wrap the existing calls directly to Mock, and will be responsible for suppressing deprecation warnings so that the non-Mock code which currently depends on Mock can remain in-tact as much as possible, until Mock is actually removed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user ctubbsii opened a pull request:

          https://github.com/apache/accumulo/pull/40

          ACCUMULO-3920 Deprecate mock components

          == Done so far ==

          • Deprecate Accumulo Mock classes
          • Add javadoc to mock package to add additional details
          • Create `DeprecationUtil` to help migrate code which must continue to use mock
          • Minimize mock usage throughout the tests wherever possible
          • Use `EasyMock` in a few cases where it was possible to satisfy the test
          • Apply `@Rule`, `@Before`, etc. to reduce number of calls to `MockInstance`
          • Create replacement `IteratorAdapter` for use outside of mock

          == Could use help with ==
          (most of these can be pretty readily identified in Eclipse with deprecation warnings shown)

          • Migrating `FormatterCommandTest.java` to avoid use of `MockShell`
          • Migrating MapReduce tests and other "heavyweight" tests to use `MiniAccumuloCluster`
          • Migrating other "lightweight" tests to use `EasyMock`

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/ctubbsii/accumulo ACCUMULO-3920

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/accumulo/pull/40.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 #40


          commit 048f56cf6340dbcee5eec154753734bebe3bc45f
          Author: Christopher Tubbs <ctubbsii@apache.org>
          Date: 2015-07-09T02:02:03Z

          ACCUMULO-3920 Deprecate mock components

          • Deprecate Accumulo Mock classes
          • Add javadoc to mock package to add additional details
          • Create DeprecationUtil to help migrate code which must continue to use mock
          • Minimize mock usage throughout the tests wherever possible
          • Use EasyMock in a few cases where it was possible to satisfy the test
          • Apply @Rule, @Before, etc. to reduce number of calls to MockInstance
          • Create replacement IteratorAdapter for use outside of mock

          Show
          githubbot ASF GitHub Bot added a comment - GitHub user ctubbsii opened a pull request: https://github.com/apache/accumulo/pull/40 ACCUMULO-3920 Deprecate mock components == Done so far == Deprecate Accumulo Mock classes Add javadoc to mock package to add additional details Create `DeprecationUtil` to help migrate code which must continue to use mock Minimize mock usage throughout the tests wherever possible Use `EasyMock` in a few cases where it was possible to satisfy the test Apply `@Rule`, `@Before`, etc. to reduce number of calls to `MockInstance` Create replacement `IteratorAdapter` for use outside of mock == Could use help with == (most of these can be pretty readily identified in Eclipse with deprecation warnings shown) Migrating `FormatterCommandTest.java` to avoid use of `MockShell` Migrating MapReduce tests and other "heavyweight" tests to use `MiniAccumuloCluster` Migrating other "lightweight" tests to use `EasyMock` You can merge this pull request into a Git repository by running: $ git pull https://github.com/ctubbsii/accumulo ACCUMULO-3920 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/accumulo/pull/40.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 #40 commit 048f56cf6340dbcee5eec154753734bebe3bc45f Author: Christopher Tubbs <ctubbsii@apache.org> Date: 2015-07-09T02:02:03Z ACCUMULO-3920 Deprecate mock components Deprecate Accumulo Mock classes Add javadoc to mock package to add additional details Create DeprecationUtil to help migrate code which must continue to use mock Minimize mock usage throughout the tests wherever possible Use EasyMock in a few cases where it was possible to satisfy the test Apply @Rule, @Before, etc. to reduce number of calls to MockInstance Create replacement IteratorAdapter for use outside of mock
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user keith-turner commented on the pull request:

          https://github.com/apache/accumulo/pull/40#issuecomment-120941438

          I am thinking we can drop `IntersectingIteratorTest.testWithBatchScanner()` because `ExamplesIT.testShardedIndex()` will test IntersectingIter using a real batch scanner in an IT. `IntersectingIteratorTest.testWithBatchScanner()` is testing IntersectingIter using the mock batch scanner. Alternatively if we want to treat this as a test of mock, we could just suppress warnings on the test and add a comment stating the test is intended to test the mock batch scanner.

          Show
          githubbot ASF GitHub Bot added a comment - Github user keith-turner commented on the pull request: https://github.com/apache/accumulo/pull/40#issuecomment-120941438 I am thinking we can drop `IntersectingIteratorTest.testWithBatchScanner()` because `ExamplesIT.testShardedIndex()` will test IntersectingIter using a real batch scanner in an IT. `IntersectingIteratorTest.testWithBatchScanner()` is testing IntersectingIter using the mock batch scanner. Alternatively if we want to treat this as a test of mock, we could just suppress warnings on the test and add a comment stating the test is intended to test the mock batch scanner.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ctubbsii commented on the pull request:

          https://github.com/apache/accumulo/pull/40#issuecomment-122049374

          Works for me. Let's drop the redundant test.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ctubbsii commented on the pull request: https://github.com/apache/accumulo/pull/40#issuecomment-122049374 Works for me. Let's drop the redundant test.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user keith-turner commented on the pull request:

          https://github.com/apache/accumulo/pull/40#issuecomment-122291984

          I removed `IntersectingIteratorTest.testWithBatchScanner() ` in 712f4e8. I checked coverge of `IntersectingIterator` before and after. It went from 73.8 to 73.6

          Show
          githubbot ASF GitHub Bot added a comment - Github user keith-turner commented on the pull request: https://github.com/apache/accumulo/pull/40#issuecomment-122291984 I removed `IntersectingIteratorTest.testWithBatchScanner() ` in 712f4e8. I checked coverge of `IntersectingIterator` before and after. It went from 73.8 to 73.6
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/accumulo/pull/40

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/accumulo/pull/40

            People

            • Assignee:
              ctubbsii Christopher Tubbs
              Reporter:
              ctubbsii Christopher Tubbs
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0h
                0h
                Logged:
                Time Spent - 40m
                40m

                  Development