Cleaning up tests

Some steps I typically follow in cleaning up the test:

1)  Rename the test
    Commit the file rename as its own commit 
   
2)  Move all constants to top of test class
    Move all fields to top of test class
    Move all inner-classes to bottom of test class
    Move all Rules below fields
    Move all setUp/tearDown methods below Rules
    Move all test methods below setUp/tearDown methods
    Move all other methods below test methods

    All fields, vars, etc should be defined alone on a dedicated line

3)  Reduce scope of everything to small as possible (ex: private)
    * Rules must be defined public
    * Mockito @Mock fields can be defined private

4)  Update use of deprecated APIs with current Geode APIs

5)  Convert JUnit assertions to AssertJ
    * I use regex find/replace in IntelliJ
    * See https://joel-costigliola.github.io/assertj/assertj-core-converting-junit-assertions-to-assertj.html
    * Review them and adjust as needed
    * Convert if-then-fail blocks to use assertions

6)  Convert multiple variations of the same test to JUnitParams
    (Usually two tests both calling same doTest method with one or more parameters)

    * Add @RunWith(JUnitParamsRunner.class) at top of test class
    * Adjust one or more test method to use JUnitParams:

    From:
        @Test
        public void test1CCEServer() {
          doServerTest(true);
        }
        @Test
        public void test2NormalServer() {
          doServerTest(false);
        }
        private void doServerTest(boolean CCE) {
          ...
        }

    To:
        @Test
        @Parameters({"true", "false"})
        @TestCaseName("{method}(concurrencyChecksEnabled={0})")
        public void clientReceivesKeysRegisteredFor(boolean concurrencyChecksEnabled) {
          ...
        }

7)  Change from using helper layers to directly using Geode APIs
    * Replace rules and helpers with direct usage of Geode API

8)  Rename AsyncInvocation instances to indicate what and where

    AsyncInvocation createCqInClient1 = client1.invokeAsync
   
    Notes:
    * what = create continuous query (cq)
    * where = in client1

9)  Use AsyncInvocation.await() instead of .join(), .exceptionOccurred, or .getException()

    createCqInClient1.await()
   
    Notes:
    * does all of the above AND uses the GeodeAwaitility timeout

10) Convert from WaitCriterion with assertion to Awaitility (GeodeAwaitility) with assertion

11) Convert from non-assertion WaitCriterion to Awaitility (GeodeAwaitility)

    From:
        WaitCriterion ev = new WaitCriterion() {
          @Override
          public boolean done() {
            Entry entry = region.getEntry(key);
            return entry != null && entry.getValue() == null;
          }

          @Override
          public String description() {
            return null;
          }
        };
        Wait.waitForCriterion(ev, 10 * 1000, 1000, true);

    To:
        await("entry with null value exists for " + key).until(() -> {
          Entry entry = region.getEntry(key);
          return entry != null && entry.getValue() == null;
        });
       
    Or:
        await("entry with null value exists for " + key).untilAsserted(() -> {
          assertThat(region.getEntry(key).getValue()).isNull();
        });
   
    Notes:
    * untilAsserted catches NPEs and keeps retrying until the timeout

12) Inline super-class(es) into current test class without deleting super-class(es)
    * Delete everything that is not used (repeat until done)
    * Inline all super classes except DistributedTestCase or CacheTestCase if those are used

13) Inline helper methods (especially small ones)

    From:
        public void myTest() {
          closeCache();
        ...
        private void closeCache() {
          vm.invoke(() -> getCache().close());
        }

    To:
        public void myTest() {
          vm.invoke(() -> getCache().close());

14) Transpose VM parameters on a method to just having the VM invoke the method

    From:
        public void myTest() {
          createCache(server2);
        ...
        private void createCache(VM vm) {
          vm.invoke(() -> {
            // many lines to create cache, diskstores, regions, etc.
          });
        }

    To:
        public void myTest() {
          server2.invoke(() -> createCache());
        ...
        private void createCache() {
          // many lines to create cache, diskstores, regions, etc.
        }

15) Remove all catch-blocks and fail statements for unexpected exceptions
    * Delete the try/catch with fail/throw
    * Add all checked exceptions to method signatures
    * Do not use "throws Throwable" or "throws Error" in tests

    From:
        public/private void method() {
          ...
          try {
            // do something that may throw checked exception
          } catch (Exception e) {
            fail(e);
            // or:
            throw new AssertionError(e);
          }
          ...
        }
        
    To:
        public/private void method() throws IOException {
          ...
          // do something that may throw checked exception
          ...
        }

16) Replace try-fail-catch pattern for expected exception with catchThrowable or assertThatThrownBy

    From:
        try {
          doPutAll(regionName, title, numberOfEntries);
          fail("expected to throw");
        } catch (ServerOperationException e) {
          assertTrue(e.getMessage().contains("putAll at server applied partial keys due to exception"));
        }

    To:
        Throwable thrown = catchThrowable(() -> doPutAll(regionName, title, numberOfEntries));
        assertThat(thrown)
            .isInstanceOf(ServerOperationException.class)
            .hasMessageContaining("putAll at server applied partial keys due to exception");

17) Use description test method naming

18) Remove atMost from uses of GeodeAwaitility to ensure full timeout of 5 minutes is used

19) Update or fix object wait/notify usage
    * Awaitility is usually much better

20) Change thread-unsafe callback (listener) fields to volatile or Atomic instances

21) Avoid use of assertThat(region).hasSize(n)

    Do use this syntax for EVERY collection type exception for Region.

    AssertJ treads Region as a collection and will walk it to generate the failure message if
    the assertion fails. This can result in messaging and even hangs, especially if the Region
    is a PartitionedRegion.

    Examples that are ok:
        assertThat(region.getSize()).isEqualTo(10);
        assertThat(region.getSize()).isGreaterThanOrEqualTo(1);

22) Sometimes transposing VM invoke with assertion can look or work better one way or the other

    From:
        assertThat(getRegionSize(server1, regionName)).isEqualTo(0);

    To:
        server1.invoke(() -> {
          assertThat(getRegionSize(regionName)).isEqualTo(0);
        })

    To:
        server1.invoke(() -> {
          assertThat(getCache().getRegion(regionName).size()).as("region size").isEqualTo(0);
        })

23) Convert SerializableRunnable/SerializableCallable fields to methods and invoke from lambda

    From:
        private SerializableRunnable doSomethingInteresting = new SerializableRunnable("doSomethingInteresting") {
          @Override
          public void run() {
            // do something
          }
        };

        clientVM.invoke(doSomethingInteresting);

   To:
        private void doSomethingInteresting() {
          // do something
        }

        clientVM.invoke(() -> doSomethingInteresting());