test: ITStorage cleanup

[dmitry-fa]

The following changes have been done:

• testListBlobRequesterPays, testUpdateBucketRequesterPays - make independent from other tests
• testAttemptDeletionObjectTemporaryHold - restoring operation corrected
• testBucketAcl, testUpdateBucketRequesterPays , testBucketPolicyV1RequesterPays - updated to use the dedicated bucket
• @before code now is invoked only by a few tests, not all

Fixes: #409

[codecov]

Codecov Report

Merging #439 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #439   +/-   ##
=========================================
  Coverage     63.15%   63.15%           
- Complexity      610      619    +9     
=========================================
  Files            32       32           
  Lines          5133     5133           
  Branches        490      489    -1     
=========================================
  Hits           3242     3242           
  Misses         1726     1726           
  Partials        165      165           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e63feb7...dc47720. Read the comment docs.

[frankyn]

Why not leave as-is with Before annotation? Otherwise it can be easily missed when new requester pays tests are added.

[dmitry-fa]

@frankyn
The purpose of @Before annotation here is to invoke the method to mend the shared BUCKET after requester-pays tests before each test. With this cleanup fix, all the requester-pays tests are "moved" to their dedicated folder, it's no longer required to mend the BUCKET.

Actually, it's not absolutely necessary to invoke unsetRequesterPays before requester-pays tests, these tests will work without it. Invocation is only needed to make setRequesterPays(true) change the value from not true to true. So it's not critical if a new requester-pays test misses calling unsetRequesterPays().

[dmitry-fa]

Generic question. I wrote a utility to run IT cases individually. With a very small change, it could be applied not to ITStorageTest but to any IT test based on JUnit. Not a masterpiece, but could be helpful in the future. Does it make sense to integrate this somewhere:

package com.google.cloud.storage.it;

import java.io.IOException;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import org.junit.Test;
import org.junit.runner.JUnitCore;
import org.junit.runner.Request;
import org.junit.runner.Result;

/**
 * Utility class to run all IT Storage test cases individually. This class extracts the existing
 * case names from {@link ITStorageTest} and launches a new JVM for each case.
 *
 * <p>Running tests individually allows to detect tests which rely on the effect from
 * running other tests.
 *
 * <p>To run this utility just compile and run this class with the same classpath as you run
 * {@link ITStorageTest}. Two environment variables are required to be set:
 * {@code GOOGLE_APPLICATION_CREDENTIALS} and  {@code IT_SERVICE_ACCOUNT_EMAIL}
 */
public class RunIndividually {

  static final Class IT_CLASS = ITStorageTest.class;

  static final List<String> MANDATORY =
      Arrays.asList("GOOGLE_APPLICATION_CREDENTIALS", "IT_SERVICE_ACCOUNT_EMAIL");

  public static void main(String... args) throws InterruptedException, IOException {
    for (String var : MANDATORY) {
      if (System.getenv(var) == null) {
        System.err.println("Mandatory environment variable '" + var + "' is not set.");
        System.exit(1);
      }
    }

    if (args.length == 0) {
      String separator = System.getProperty("file.separator");
      String classpath = System.getProperty("java.class.path");
      String path = System.getProperty("java.home") + separator + "bin" + separator + "java";

      List<String> tests = getTestList();
      List<String> failed = new ArrayList<>();
      for (int i = 0; i < tests.size(); i++) {
        String test = tests.get(i);
        System.out.println("#" + (i + 1) + " of " + tests.size());
        ProcessBuilder processBuilder =
            new ProcessBuilder(path, "-cp", classpath, RunIndividually.class.getName(), test)
                .redirectOutput(ProcessBuilder.Redirect.INHERIT)
                .redirectError(ProcessBuilder.Redirect.INHERIT);
        int exitCode = processBuilder.start().waitFor();
        if (exitCode != 0) {
          failed.add(test);
        }
      }
      System.out.println(failed);
      System.out.println("Total failed " + failed.size());
    } else {
      JUnitCore core = new JUnitCore();
      for (String test : args) {
        System.out.println("----- " + test + " ------ ");
        Request request = Request.method(IT_CLASS, test);
        Result result = core.run(request);
        System.out.println("### " + test + " --> " + result.wasSuccessful());
        if (!result.wasSuccessful()) {
          System.out.println(result.getFailures().get(0).getMessage());
          System.exit(97);
        }
      }
    }
  }

  static List<String> getTestList() {
    List<String> tests = new ArrayList<>();
    for (Method method : IT_CLASS.getDeclaredMethods()) {
      if (method.isAnnotationPresent(Test.class)) {
        tests.add(method.getName());
      }
    }
    return tests;
  }
}

[elharo]

Why do you think the test methods are connected? Where is the shared state?

[elharo]

You might simply need more instance variables and fewer static ones. BUCKET should probably be an instance variable.

[dmitry-fa]

BUCKET is a constant standing for a bucket name shared across multiple tests. There is no problem with sharing variables within the VM, some but cases might intersect using a shared resource in the Storage.

[elharo]

yes, and that's why we should use a fresh bucket for each test. I think getting a new name each time will suffice.

[dmitry-fa]

Yes, a fresh bucket per test will certainly help, but I'm not sure we can afford to create so many buckets. ITStorageTest often fails because of exceeded rate limit to bucket creation

[dmitry-fa]

@elharo

Why do you think the test methods are connected? Where is the shared state?

Test methods should not be connected, but sometimes there are. I found two storage tests that checked that a bucket property set to false, they passed when were run in the bundle because some other tests set this property to false. Being run individually they failed because that property was set to null.

There could be other non-trivial dependencies between tests which not easy to catch. They could start failing unexpectedly when the test execution order changes.

[elharo]

@dmitry-fa This is a real YAGNI. If the test methods are coupled, that's what needs to be fixed. Starting up a new VM for each test is way too heavyweight and still won't address the most likely case of test coupling here: shared tests resources outside the VM in the filesystem or GCS.

[dmitry-fa]

@elharo The suggested utility is not a replacement of the current test execution, it's a tool to discover the coupled tests. It could be run from time to time. And my question does it make sense to integrate it somewhere?

[frankyn]

Current changes look reasonable to me. I'm not sure about the additional utility though. This also fixes a mismatch between temporary and eventbased hold in a clean up step.