unittests should compare result of kustomize build to golden set of Y…

…AML resources. (#1016)

* unittests should compare result of kustomize build to golden set of YAML resources.

* Per #306 to allow reviewers to verify that the expected
  output is correct we should check in the result of "kustomize build -o"
  so that reviewers can review the diff and verify that it is correct.

* This also simplifies the test generation code; the python script
  generate_tests.py just recourses over the directory tree and runs "kustomize build -o" and checks in the output into the test_data directory.

* This is different from what the tests are currently doing.

  * Currently what the generation scripts do is generate "kustomization.yaml" files and then generate the expected output from that when the test is run.

  * This makes it very difficult to validate the expected output and to
    debug whether the expected output is correct.

  * Going forward, per #1014, I think what we want to do is check in test cases
    corresponding to kustomization.yaml files corresponding to various
    kustomizations that we want to validate are working correctly

  * Our generate scripts would then run "kustomize build" to generate expected
    output and check that in so that we can validate that the expected output
    is correct.

* Also change the tests data structure so that it mirrors the kustomize directory tree rather than flattening the tests into the "tests" directory.

  * Fix #683

* Right now running the unittests takes a long time

  * The problem is that we generate unittests for every "kustomization.yaml"
    file
  * Per #1014 this is kind of pointless/redundant because most of these
    tests aren't actually testing kustomizations.
  * We will address this in follow on PRs which will add more appropriate
    tests and remove some of these unnecessary/redundant tests.

* Cherry pick AWS fixes.

* Regenerate the tests.

* Fix the unittests; need to update the generate logic to remove unused tests
 to remove tests that aren't part of this PR.

* Address comments.

* Rebase on master and regenerate the tests.

hack/generate_tests.py

@@ -2,26 +2,54 @@
 """Regenerate tests for only the files that have changed."""
 
 import argparse
+import jinja2
 import logging
 import os
+import shutil
 import subprocess
 
-TOP_LEVEL_EXCLUDES = ["docs", "hack", "tests", "kfdef"]
+TOP_LEVEL_EXCLUDES = ["docs", "hack", "kfdef", "stacks", "tests"]
 
-def generate_test_name(repo_root, package_dir):
-  """Generate the name of the go file to write the test to.
+# The subdirectory to story the expected manifests in
+# We use a subdirectory of test_data because we could potentially
+# have more than one version of a manifest.
+KUSTOMIZE_OUTPUT_DIR = "test_data/expected"
+
+TEST_NAME = "kustomize_test.go"
+
+def generate_test_path(repo_root, kustomize_rpath):
+  """Generate the full path of the  test.go file for a particular package
 
   Args:
     repo_root: Root of the repository
-    package_dir: Full path to the kustomize directory.
+    kustomize_rpath: The relative path (relative to repo root) of the
+      kustomize package to generate the test for.
   """
 
+  test_path = os.path.join(repo_root, "tests", kustomize_rpath,
+                           TEST_NAME)
+  return test_path
+
+def run_kustomize_build(repo_root, package_dir):
+  """Run kustomize build and store the output in the test directory."""
+
   rpath = os.path.relpath(package_dir, repo_root)
 
-  pieces = rpath.split(os.path.sep)
-  name = "-".join(pieces) + "_test.go"
+  output_dir = os.path.join(repo_root, "tests", rpath, KUSTOMIZE_OUTPUT_DIR)
 
-  return name
+  if os.path.exists(output_dir):
+    # Remove any previous version of the directory so that we ensure
+    # that all files in that directory are from the new run
+    # of kustomize build -o
+    logging.info("Removing directory %s", output_dir)
+    shutil.rmtree(output_dir)
+
+  logging.info("Creating directory %s", output_dir)
+  os.makedirs(output_dir)
+
+  subprocess.check_call(["kustomize", "build", "--load_restrictor", "none",
+                         "-o", output_dir], cwd=os.path.join(repo_root,
+                                                             package_dir))
 
 def get_changed_dirs():
   """Return a list of directories of changed kustomization packages."""
@@ -93,25 +121,64 @@ def remove_unmatched_tests(repo_root, package_dirs):
   expected_tests = set()
 
   for d in package_dirs:
-    expected_tests.add(generate_test_name(repo_root, d))
+    rpath = os.path.relpath(d, repo_root)
+    expected_tests.add(generate_test_path(repo_root, rpath))
 
   tests_dir = os.path.join(repo_root, "tests")
-  for name in os.listdir(tests_dir):
-    if not name.endswith("_test.go"):
+
+  possible_empty_dirs = []
+
+  # Walk the tests directory
+  for root, dirs, files in os.walk(tests_dir):
+    if not files:
+      possible_empty_dirs.append(root)
+
+    for f in files:
+      if f != TEST_NAME:
+        continue
+
+      full_test = os.path.join(root, f)
+      if full_test not in expected_tests:
+       logging.info("Removing unexpected test: %s", full_test)
+       os.unlink(full_test)
+       possible_empty_dirs.append(root)
+
+  # Prune directories that only contain test_data but no more tests
+  for d in possible_empty_dirs:
+    if not os.path.exists(d):
+      # Might have been a subdirectory of a directory that was deleted.
+      continue
+
+    items = os.listdir(d)
+
+    if len(items) > 1:
       continue
 
-    # TODO(jlewi): we should rename and remove "_test.go" from the filename.
-    if name in ["kusttestharness_test.go"]:
-      logging.info("Ignoring %s", name)
+    if len(items) == 1 and items[0] != "test_data":
       continue
 
-    if not name in expected_tests:
-      test_file = os.path.join(tests_dir, name)
-      logging.info("Deleting test %s; doesn't map to kustomization file",
-                   test_file)
-      os.unlink(test_file)
+    logging.info("Removing directory: %s", d)
+    shutil.rmtree(d)
+
+def write_go_test(test_path, package_name, package_dir):
+  """Write the go test file.
+
+  Args:
+    test_path: Path for the go file
+    package_name: The name for the go package the test should live in
+    package_dir: The path to the kustomize package being tested; this
+      should be the relative path to the kustomize directory.
+  """
+  test_contents = template.render({"package": package_name,
+                                   "package_dir":package_dir})
+
+
+  logging.info("Writing file: %s", test_path)
+  with open(test_path, "w") as test_file:
+    test_file.write(test_contents)
 
 if __name__ == "__main__":
+
   logging.basicConfig(
       level=logging.INFO,
       format=('%(levelname)s|%(asctime)s'
@@ -146,12 +213,41 @@ def remove_unmatched_tests(repo_root, package_dirs):
   else:
     changed_dirs = get_changed_dirs()
 
+  this_dir = os.path.dirname(__file__)
+  loader = jinja2.FileSystemLoader(searchpath=os.path.join(
+    this_dir, "templates"))
+  env = jinja2.Environment(loader=loader)
+  template = env.get_template("kustomize_test.go.template")
+
   for full_dir in changed_dirs:
-    test_name = generate_test_name(repo_root, full_dir)
-    logging.info("Regenerating test %s for %s ", test_name, full_dir)
+    # Get the relative path of the kustomize directory.
+    # This is the path relative to the repo root.
+    rpath = os.path.relpath(full_dir, repo_root)
+
+    test_path = generate_test_path(repo_root, rpath)
+    logging.info("Regenerating test %s for %s ", test_path, full_dir)
+
+    # Generate the kustomize output
+    run_kustomize_build(repo_root, full_dir)
+
+    # Create the go test file.
+    # TODO(jlewi): We really shouldn't need to redo this if it already
+    # exists.
+
+    # The go package name will be the final directory in the path
+    package_name = os.path.basename(full_dir)
+    # Go package names replace hyphens with underscores
+    package_name = package_name.replace("-", "_")
+
+    # We need to construct the path relative to the _test.go file of
+    # the kustomize package. This path with consist of ".." entries repeated
+    # enough times to get to the root of the repo. We then add the relative
+    # path to the kustomize package.
+    pieces = rpath.split(os.path.sep)
 
-    test_path = os.path.join(repo_root, "tests", test_name)
+    p = [".."] * len(pieces)
+    p.append("..")
+    p.append(rpath)
+    package_dir = os.path.join(*p)
 
-    with open(test_path, "w") as test_file:
-      subprocess.check_call(["./hack/gen-test-target.sh", full_dir],
-                            stdout=test_file, cwd=repo_root)
+    write_go_test(test_path, package_name, package_dir)

hack/templates/kustomize_test.go.template

@@ -0,0 +1,15 @@
+package {{package}}
+
+import (
+	"github.com/kubeflow/manifests/tests"
+	"testing"
+)
+
+func TestKustomize(t *testing.T) {
+	testCase := &tests.KustomizeTestCase{
+		Package: "{{package_dir}}",
+		Expected: "test_data/expected",
+	}
+
+	tests.RunTestCase(t, testCase)
+}

tests/Makefile

@@ -35,7 +35,7 @@ modules:
 	@GO111MODULE=on $(GO) mod download
 
 test: modules
-	@GO111MODULE=on $(GO) test -v .
+	@GO111MODULE=on $(GO) test -v github.com/kubeflow/manifests/tests/...
 
 clean:
 	@rm -f $$(ls *_test.go | egrep -v 'kusttestharness_test.go|go.mod|go.sum')