Skip to content

Commit

Permalink
Remove unit test runtime dependencies on resources (pantsbuild#8066)
Browse files Browse the repository at this point in the history
### Problem
Currently, `test_base.py`, `jvm_tool_task_test_base.py`, and `interpreter_cache_test_mixin.py` have implicit runtime dependencies on files defined at the build root, specifically `./pants`, `./BUILD.tools`, `3rdparty/BUILD`, and `.pants.d`. 

Because V2 is completely hermetic, these runtime dependencies fail to resolve as the test runner does not know to copy the files into the snapshot. Instead, we need some way to explicitly mark these dependencies via the `BUILD` file or to remove the dependency outright.

#### FYI: why didn't we detect this earlier?
We've been using V2 to run unit tests in CI for the past two months. Why didn't we detect this? The reason is that V2 runs locally in `.pantsd`, and the logic to get the buildroot traverses ancestors.

This was only detected when running unit tests with remote execution.

### Solution
* Remove the dependency on `./pants`, which was not actually needed. 
* Explicitly depend on `./BUILD.tools` and `3rdparty/BUILD`. 
* Remove `interpreter_cache_test_mixin.py`, which is inherently non-hermetic and cannot work with the V2 model. Even though this has a slight performance impact, this is offset by the remote caching we will now be able to get + having more sound / hermitic tests.
  • Loading branch information
Eric-Arellano authored and stuhood committed Jul 18, 2019
1 parent 514814a commit da57bce
Show file tree
Hide file tree
Showing 9 changed files with 22 additions and 42 deletions.
12 changes: 12 additions & 0 deletions BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

files(
name = 'build_tools',
source = 'BUILD.tools',
)

files(
name = '3rdparty_build',
source = '3rdparty/BUILD',
)
1 change: 0 additions & 1 deletion tests/python/pants_test/backend/project_info/tasks/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ python_tests(
'src/python/pants/util:osutil',
'tests/python/pants_test/subsystem:subsystem_utils',
'tests/python/pants_test:task_test_base',
'tests/python/pants_test/backend/python/tasks:interpreter_cache_test_mixin',
]
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,11 @@
from pants.util.contextutil import temporary_dir
from pants.util.dirutil import chmod_plus_x, safe_open
from pants.util.osutil import get_os_name, normalize_os_name
from pants_test.backend.python.tasks.interpreter_cache_test_mixin import InterpreterCacheTestMixin
from pants_test.subsystem.subsystem_util import init_subsystems
from pants_test.task_test_base import ConsoleTaskTestBase


class ExportTest(InterpreterCacheTestMixin, ConsoleTaskTestBase):
class ExportTest(ConsoleTaskTestBase):
@classmethod
def task_type(cls):
return Export
Expand Down
6 changes: 0 additions & 6 deletions tests/python/pants_test/backend/python/tasks/BUILD
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

python_library(
name='interpreter_cache_test_mixin',
sources=['interpreter_cache_test_mixin.py'],
)

python_library(
name='python_task_test_base',
sources=['python_task_test_base.py'],
dependencies=[
':interpreter_cache_test_mixin',
'src/python/pants/backend/python:plugin',
'src/python/pants/backend/python/targets',
'src/python/pants/build_graph',
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
from pants.backend.python.register import build_file_aliases as register_python
from pants.backend.python.targets.python_binary import PythonBinary
from pants.build_graph.address import Address
from pants_test.backend.python.tasks.interpreter_cache_test_mixin import InterpreterCacheTestMixin
from pants_test.subsystem import subsystem_util
from pants_test.task_test_base import TaskTestBase

Expand All @@ -35,7 +34,7 @@ def normalized_current_platform():
return normalize_platform_tag(pep425tags.get_platform())


class PythonTaskTestBase(InterpreterCacheTestMixin, TaskTestBase):
class PythonTaskTestBase(TaskTestBase):
"""
:API: public
"""
Expand Down
3 changes: 3 additions & 0 deletions tests/python/pants_test/jvm/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ python_library(
name='jvm_tool_task_test_base',
sources=['jvm_tool_task_test_base.py'],
dependencies=[
'//:build_tools',
'//:3rdparty_build',
':jvm_task_test_base',
'src/python/pants/backend/jvm/subsystems:jvm_tool_mixin',
'src/python/pants/backend/jvm/targets:jvm',
Expand All @@ -33,6 +35,7 @@ python_library(
'src/python/pants/build_graph',
'src/python/pants/ivy',
'src/python/pants/java/jar',
'src/python/pants/util:dirutil',
]
)

Expand Down
11 changes: 5 additions & 6 deletions tests/python/pants_test/jvm/jvm_tool_task_test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,11 @@ def setUp(self):
read_from=artifact_caches,
write_to=artifact_caches)

# Tool option defaults currently point to targets in the real BUILD.tools, so we copy it and
# its dependency BUILD files into our test workspace.
shutil.copy(os.path.join(self.real_build_root, 'BUILD.tools'), self.build_root)
third_party = os.path.join(self.build_root, '3rdparty')
safe_mkdir(third_party)
shutil.copy(os.path.join(self.real_build_root, '3rdparty', 'BUILD'), third_party)
# Copy into synthetic build-root
shutil.copy('BUILD.tools', self.build_root)
build_root_third_party = os.path.join(self.build_root, '3rdparty')
safe_mkdir(build_root_third_party)
shutil.copy(os.path.join('3rdparty', 'BUILD'), build_root_third_party)

Bootstrapper.reset_instance()

Expand Down
1 change: 0 additions & 1 deletion tests/python/pants_test/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,6 @@ def pants_workdir(self):
@classmethod
@memoized_method
def _build_root(cls):
cls.real_build_root = BuildRoot().path
return os.path.realpath(mkdtemp(suffix='_BUILD_ROOT'))

@classmethod
Expand Down

0 comments on commit da57bce

Please sign in to comment.