Properly strip source root prefixes for V2 Pytest runner (pantsbuild…

…#8185) ### Problem pantsbuild#7696 introduced support for source roots to V2 Pytest for the first time, but it did not support loose files. pantsbuild#8063 tried to fix this by no longer stripping the source root from source files. However, this caused two issues: 1) Regression that `repr()` now shows the full path, not the relativized path: pantsbuild#8063 (comment) 2) Namespace packages, such as contrib code, do not work with V2. A namespace package is where multiple folders have the same package name, like `pants`, but may not within the same actual folder. * Contrib unit tests would fail because `PYTHONPATH` had two entries referring to `pants`: `src/python/pants` and `contrib/mypy/src/python/pants`. Python would use whichever entry came first and ignore the other, which does not work. Instead, we want those two to merge into one namespace package. ### Solution Follow the V1 Pytest approach of stripping the source root, like we used to, but only for source files. Loose files (i.e. `files()`) are not stripped so that the file system APIs work as expected. This approach comes from: https://github.com/pantsbuild/pants/blob/d048fd2e8f34adc32fdce36a51a765fbf6067cff/src/python/pants/backend/python/subsystems/pex_build_util.py#L101-L110 ### Result V2 Pytest now supports both source roots and loose files! This allows us to run most contrib unit tests with V2 and unblocks pantsbuild#8113. Because over 99% of unit tests are now remoted, we go back to only one CI shard for unit tests.
anthonyjpratti · Aug 25, 2019 · 538b976 · 538b976
1 parent a8f3ccd
commit 538b976
Show file tree

Hide file tree

Showing 15 changed files with 140 additions and 316 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -381,17 +381,17 @@ matrix:
     - BOOTSTRAPPED_PEX_KEY_SUFFIX=py37.linux
     - PANTS_REMOTE_CA_CERTS_PATH=/usr/lib/google-cloud-sdk/lib/third_party/grpc/_cython/_credentials/roots.pem
     - PANTS_NATIVE_BUILD_STEP_CPP_COMPILE_SETTINGS_DEFAULT_COMPILER_OPTION_SETS="[]"
-    - CACHE_NAME=unit_tests.v2.py37
+    - CACHE_NAME=unit_tests.py37
     language: python
-    name: Unit tests - V2 test runner (Python 3.7)
+    name: Unit tests (Python 3.7)
     os: linux
     python:
     - '2.7'
     - '3.6'
     - '3.7'
     script:
-    - travis_wait 50 ./build-support/bin/ci.py --python-tests-v2 --remote-execution-enabled
-      --python-version 3.7
+    - travis_wait 50 ./build-support/bin/ci.py --unit-tests --remote-execution-enabled
+      --plugin-tests --python-version 3.7
     stage: Test Pants (Cron)
     sudo: required
   - addons:
@@ -614,134 +614,19 @@ matrix:
     env:
     - BOOTSTRAPPED_PEX_KEY_SUFFIX=py36.linux
     - PANTS_REMOTE_CA_CERTS_PATH=/usr/lib/google-cloud-sdk/lib/third_party/grpc/_cython/_credentials/roots.pem
-    - CACHE_NAME=unit_tests.v2.py36
+    - CACHE_NAME=unit_tests.py36
     language: python
-    name: Unit tests - V2 test runner (Python 3.6)
+    name: Unit tests (Python 3.6)
     os: linux
     python:
     - '2.7'
     - '3.6'
     - '3.7'
     script:
-    - travis_wait 50 ./build-support/bin/ci.py --python-tests-v2 --remote-execution-enabled
-      --python-version 3.6
+    - travis_wait 50 ./build-support/bin/ci.py --unit-tests --remote-execution-enabled
+      --plugin-tests --python-version 3.6
     stage: Test Pants
     sudo: required
-  - addons:
-      apt:
-        packages:
-        - lib32stdc++6
-        - lib32z1
-        - lib32z1-dev
-        - gcc-multilib
-        - python-dev
-        - openssl
-        - libssl-dev
-        - jq
-        - unzip
-        - shellcheck
-    after_failure:
-    - ./build-support/bin/ci-failure.sh
-    before_cache:
-    - sudo chown -R travis:travis "${HOME}" "${TRAVIS_BUILD_DIR}"
-    - find ${HOME}/.ivy2/pants -type f -name "ivydata-*.properties" -delete
-    - rm -f ${HOME}/.ivy2/pants/*.{css,properties,xml,xsl}
-    - rm -rf ${HOME}/.ivy2/pants/com.example
-    - du -m -d2 ${HOME}/.cache/pants | sort -r -n
-    before_install:
-    - PATH="/usr/lib/jvm/java-8-openjdk-amd64/jre/bin":$PATH
-    - JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64
-    - sudo sysctl fs.inotify.max_user_watches=524288
-    - ./build-support/bin/install_aws_cli_for_ci.sh
-    - pyenv global 2.7.15 3.6.7 3.7.1
-    before_script:
-    - ./build-support/bin/get_ci_bootstrapped_pants_pex.sh ${BOOTSTRAPPED_PEX_BUCKET}
-      ${BOOTSTRAPPED_PEX_KEY_PREFIX}.${BOOTSTRAPPED_PEX_KEY_SUFFIX}
-    cache:
-      directories:
-      - ${AWS_CLI_ROOT}
-      - ${PYENV_ROOT}
-      - ${HOME}/.cache/pants/lmdb_store
-      - ${HOME}/.cache/pants/tools
-      - ${HOME}/.cache/pants/zinc
-      - ${HOME}/.ivy2/pants
-      - ${HOME}/.npm
-      timeout: 500
-    dist: xenial
-    env:
-    - BOOTSTRAPPED_PEX_KEY_SUFFIX=py36.linux
-    - PANTS_REMOTE_CA_CERTS_PATH=/usr/lib/google-cloud-sdk/lib/third_party/grpc/_cython/_credentials/roots.pem
-    - CACHE_NAME=unit_tests.v1.py36
-    language: python
-    name: Unit tests - V1 test runner (Python 3.6)
-    os: linux
-    python:
-    - '2.7'
-    - '3.6'
-    - '3.7'
-    script:
-    - ./build-support/bin/ci.py --python-tests-v1 --python-version 3.6
-    - ./build-support/bin/ci.py --plugin-tests --python-version 3.6
-    stage: Test Pants
-    sudo: required
-  - addons:
-      apt:
-        packages:
-        - lib32stdc++6
-        - lib32z1
-        - lib32z1-dev
-        - gcc-multilib
-        - python-dev
-        - openssl
-        - libssl-dev
-        - jq
-        - unzip
-        - shellcheck
-    after_failure:
-    - ./build-support/bin/ci-failure.sh
-    before_cache:
-    - sudo chown -R travis:travis "${HOME}" "${TRAVIS_BUILD_DIR}"
-    - find ${HOME}/.ivy2/pants -type f -name "ivydata-*.properties" -delete
-    - rm -f ${HOME}/.ivy2/pants/*.{css,properties,xml,xsl}
-    - rm -rf ${HOME}/.ivy2/pants/com.example
-    - du -m -d2 ${HOME}/.cache/pants | sort -r -n
-    before_install:
-    - PATH="/usr/lib/jvm/java-8-openjdk-amd64/jre/bin":$PATH
-    - JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64
-    - sudo sysctl fs.inotify.max_user_watches=524288
-    - ./build-support/bin/install_aws_cli_for_ci.sh
-    - pyenv global 2.7.15 3.6.7 3.7.1
-    before_script:
-    - ./build-support/bin/get_ci_bootstrapped_pants_pex.sh ${BOOTSTRAPPED_PEX_BUCKET}
-      ${BOOTSTRAPPED_PEX_KEY_PREFIX}.${BOOTSTRAPPED_PEX_KEY_SUFFIX}
-    cache:
-      directories:
-      - ${AWS_CLI_ROOT}
-      - ${PYENV_ROOT}
-      - ${HOME}/.cache/pants/lmdb_store
-      - ${HOME}/.cache/pants/tools
-      - ${HOME}/.cache/pants/zinc
-      - ${HOME}/.ivy2/pants
-      - ${HOME}/.npm
-      timeout: 500
-    dist: xenial
-    env:
-    - BOOTSTRAPPED_PEX_KEY_SUFFIX=py37.linux
-    - PANTS_REMOTE_CA_CERTS_PATH=/usr/lib/google-cloud-sdk/lib/third_party/grpc/_cython/_credentials/roots.pem
-    - PANTS_NATIVE_BUILD_STEP_CPP_COMPILE_SETTINGS_DEFAULT_COMPILER_OPTION_SETS="[]"
-    - CACHE_NAME=unit_tests.v1.py37
-    language: python
-    name: Unit tests - V1 test runner (Python 3.7)
-    os: linux
-    python:
-    - '2.7'
-    - '3.6'
-    - '3.7'
-    script:
-    - ./build-support/bin/ci.py --python-tests-v1 --python-version 3.7
-    - ./build-support/bin/ci.py --plugin-tests --python-version 3.7
-    stage: Test Pants (Cron)
-    sudo: required
   - addons:
       apt:
         packages:

diff --git a/build-support/bin/BUILD b/build-support/bin/BUILD
@@ -6,6 +6,14 @@ files(
   sources = globs('*.sh'),
 )
 
+# We include this entry, even though the scripts are already covered by individual `python_binary`
+# targets, to ensure that tests are able to treat these files as loose source files, i.e. to avoid
+# stripping the source root from the files.
+files(
+  name = 'python_scripts',
+  sources = globs('*.py'),
+)
+
 python_binary(
   name = 'check_banned_imports',
   source = 'check_banned_imports.py',

diff --git a/build-support/bin/ci.py b/build-support/bin/ci.py
@@ -37,10 +37,8 @@ def main() -> None:
     run_clippy()
   if args.cargo_audit:
     run_cargo_audit()
-  if args.python_tests_v1:
-    run_python_tests_v1()
-  if args.python_tests_v2:
-    run_python_tests_v2(remote_execution_enabled=args.remote_execution_enabled)
+  if args.unit_tests:
+    run_unit_tests(remote_execution_enabled=args.remote_execution_enabled)
   if args.rust_tests:
     run_rust_tests()
   if args.jvm_tests:
@@ -102,14 +100,9 @@ def create_parser() -> argparse.ArgumentParser:
   parser.add_argument(
     "--cargo-audit", action="store_true", help="Run Cargo audit of Rust dependencies."
   )
-  # TODO(#7772): Simplify below to always use V2 and drop the blacklist.
   parser.add_argument(
-    "--python-tests-v1", action="store_true",
-    help="Run Python unit tests with V1 test runner over the blacklist and contrib tests."
-  )
-  parser.add_argument(
-    "--python-tests-v2", action="store_true",
-    help="Run Python unit tests with V2 test runner."
+    "--unit-tests", action="store_true",
+    help="Run Python unit tests."
   )
   parser.add_argument("--rust-tests", action="store_true", help="Run Rust tests.")
   parser.add_argument("--jvm-tests", action="store_true", help="Run JVM tests.")
@@ -341,78 +334,56 @@ def run_cargo_audit() -> None:
       die("Cargo audit failure")
 
 
-def run_python_tests_v1() -> None:
+def run_unit_tests(*, remote_execution_enabled: bool) -> None:
   check_pants_pex_exists()
 
-  blacklisted_v2_targets = get_blacklisted_targets("unit_test_v2_blacklist.txt")
+  all_targets = get_all_python_tests(tag="-integration")
   blacklisted_chroot_targets = get_blacklisted_targets("unit_test_chroot_blacklist.txt")
-  chrooted_targets = blacklisted_v2_targets - blacklisted_chroot_targets
-
-  with travis_section("PythonTestsV1", "Running Python unit tests with V1 test runner"):
-
-    try:
-      subprocess.run(
-        ["./pants.pex", "--test-pytest-chroot", "test.pytest"] + sorted(chrooted_targets) + PYTEST_PASSTHRU_ARGS,
-        check=True
-      )
-      subprocess.run(
-        ["./pants.pex", "test.pytest"] + sorted(blacklisted_chroot_targets) + PYTEST_PASSTHRU_ARGS,
-        check=True
-      )
-    except subprocess.CalledProcessError:
-      die("Python unit test failure (V1 test runner")
-    else:
-      green("V1 unit tests passed.")
+  blacklisted_v2_targets = get_blacklisted_targets("unit_test_v2_blacklist.txt")
+  blacklisted_remote_targets = get_blacklisted_targets("unit_test_remote_blacklist.txt")
 
+  v1_no_chroot_targets = blacklisted_chroot_targets
+  v1_chroot_targets = blacklisted_v2_targets
+  v2_local_targets = blacklisted_remote_targets
+  v2_remote_targets = all_targets - v2_local_targets - v1_chroot_targets - v1_no_chroot_targets
 
-def run_python_tests_v2(*, remote_execution_enabled: bool) -> None:
-  check_pants_pex_exists()
+  basic_command = ["./pants.pex", "test.pytest"]
+  v2_command = ["./pants.pex", "--no-v1", "--v2", "test.pytest"]
+  v1_no_chroot_command = basic_command + sorted(v1_no_chroot_targets) + PYTEST_PASSTHRU_ARGS
+  v1_chroot_command = basic_command + ["--test-pytest-chroot"] + sorted(v1_chroot_targets) + PYTEST_PASSTHRU_ARGS
+  v2_local_command = v2_command + sorted(v2_local_targets)
 
-  blacklisted_v2_targets = get_blacklisted_targets("unit_test_v2_blacklist.txt")
-  blacklisted_remote_targets = get_blacklisted_targets("unit_test_remote_blacklist.txt")
-  all_targets = get_all_python_tests(tag="-integration")
-  v2_compatible_targets = all_targets - blacklisted_v2_targets
-  if remote_execution_enabled:
-    remote_execution_targets = v2_compatible_targets - blacklisted_remote_targets
-    local_execution_targets = blacklisted_remote_targets
+  if not remote_execution_enabled:
+    v2_local_targets = v2_local_targets | v2_remote_targets
+    v2_local_command = v2_command + sorted(v2_local_targets)
   else:
-    remote_execution_targets = set()
-    local_execution_targets = v2_compatible_targets
-
-  def run_v2_tests(
-    *, targets: Set[str], execution_strategy: str, oauth_token_path: Optional[str] = None
-  ) -> None:
-    try:
-      command = (
-        ["./pants.pex", "--no-v1", "--v2", "test.pytest"] + sorted(targets) + PYTEST_PASSTHRU_ARGS
-      )
-      if oauth_token_path is not None:
-        command[3:3] = [
+    with travis_section(
+      "UnitTestsRemote", "Running unit tests via remote execution"
+    ), get_remote_execution_oauth_token_path() as oauth_token_path:
+      v2_remote_command = v2_command[:-1] + [
           "--pants-config-files=pants.remote.ini",
           # We turn off speculation to reduce the risk of flakiness, where a test passes locally but
           # fails remoting and we have a race condition for which environment executes first.
           "--process-execution-speculation-strategy=none",
-          f"--remote-oauth-bearer-token-path={oauth_token_path}"
-        ]
-      subprocess.run(command, check=True)
+          f"--remote-oauth-bearer-token-path={oauth_token_path}",
+          "test.pytest",
+        ] + sorted(v2_remote_targets)
+      try:
+        subprocess.run(v2_remote_command, check=True)
+      except subprocess.CalledProcessError:
+        die("Unit test failure (remote execution)")
+      else:
+        green("Unit tests passed (remote execution)")
+
+  with travis_section("UnitTestsLocal", "Running unit tests via local execution"):
+    try:
+      subprocess.run(v2_local_command, check=True)
+      subprocess.run(v1_chroot_command, check=True)
+      subprocess.run(v1_no_chroot_command, check=True)
     except subprocess.CalledProcessError:
-      die(f"V2 unit tests failure ({execution_strategy} build execution).")
+      die("Unit test failure (local execution)")
     else:
-      green(f"V2 unit tests passed ({execution_strategy} build execution).")
-
-  if remote_execution_enabled:
-    with travis_section(
-      "PythonTestsV2Remote", "Running Python unit tests with V2 test runner and remote build execution"
-    ), get_remote_execution_oauth_token_path() as oauth_token_path:
-      run_v2_tests(
-        targets=remote_execution_targets,
-        execution_strategy="remote",
-        oauth_token_path=oauth_token_path
-      )
-  with travis_section(
-    "PythonTestsV2Local", "Running Python unit tests with V2 test runner and local build execution"
-  ):
-    run_v2_tests(targets=local_execution_targets, execution_strategy="local")
+      green("Unit tests passed (local execution)")
 
 
 def run_rust_tests() -> None:

diff --git a/build-support/bin/generate_travis_yml.py b/build-support/bin/generate_travis_yml.py
@@ -447,28 +447,18 @@ def cargo_audit() -> Dict:
 # Unit tests
 # -------------------------------------------------------------------------
 
-def unit_tests_v2(python_version: PythonVersion) -> Dict:
+def unit_tests(python_version: PythonVersion) -> Dict:
   shard = {
     **linux_shard(python_version=python_version),
-    "name": f"Unit tests - V2 test runner (Python {python_version.decimal})",
+    "name": f"Unit tests (Python {python_version.decimal})",
     "script": [
-      f"travis_wait 50 ./build-support/bin/ci.py --python-tests-v2 --remote-execution-enabled --python-version {python_version.decimal}",
+      (
+        "travis_wait 50 ./build-support/bin/ci.py --unit-tests --remote-execution-enabled "
+        f"--plugin-tests --python-version {python_version.decimal}"
+      )
     ],
   }
-  shard["env"] = shard.get("env", []) + [f"CACHE_NAME=unit_tests.v2.py{python_version.number}"]
-  return shard
-
-
-def unit_tests_v1(python_version: PythonVersion) -> Dict:
-  shard = {
-    **linux_shard(python_version=python_version),
-    "name": f"Unit tests - V1 test runner (Python {python_version.decimal})",
-    "script": [
-      f"./build-support/bin/ci.py --python-tests-v1 --python-version {python_version.decimal}",
-      f"./build-support/bin/ci.py --plugin-tests --python-version {python_version.decimal}",
-    ],
-  }
-  shard["env"] = shard.get("env", []) + [f"CACHE_NAME=unit_tests.v1.py{python_version.number}"]
+  shard["env"] = shard.get("env", []) + [f"CACHE_NAME=unit_tests.py{python_version.number}"]
   return shard
 
 # ----------------------------------------------------------------------
@@ -747,12 +737,11 @@ def main() -> None:
       # https://docs.google.com/document/d/1gL3D1f-AzL_LzRxWLskCpVQ2ZlB_26GTETgXkXsrpDY/edit#heading=h.akhkfdtqfpw,
       # the RBE token server will only give tokens to job number #5, so we must do this for the cron
       # job to work with remoting.
-      unit_tests_v2(PythonVersion.py37),
+      unit_tests(PythonVersion.py37),
       *[lint(v) for v in PythonVersion],
       clippy(),
       cargo_audit(),
-      unit_tests_v2(PythonVersion.py36),
-      *[unit_tests_v1(v) for v in PythonVersion],
+      unit_tests(PythonVersion.py36),
       build_wheels_linux(),
       build_wheels_osx(),
       *integration_tests(PythonVersion.py36),