Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for source roots in V2 ./pants test #7696

Merged
merged 24 commits into from
May 14, 2019
Merged

Add support for source roots in V2 ./pants test #7696

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
3cff08f
Rename source dep test to emphasize that it's a relative import
Eric-Arellano May 9, 2019
d6e8514
Add failing integration test for source dep with absolute import
Eric-Arellano May 9, 2019
2c6fc06
Rename dummy files to be more explicit what they mean
Eric-Arellano May 9, 2019
1cf60b4
Squashed commit of the following:
Eric-Arellano May 10, 2019
7225ad8
Merge branch 'master' of github.com:pantsbuild/pants into v2-pytest-a…
Eric-Arellano May 10, 2019
5c09f7f
First (failing) attempt at new rule
Eric-Arellano May 10, 2019
dbeb559
Actually feed new digest to pex
Eric-Arellano May 10, 2019
2172cbc
Relativize source files to source root (hardcoded)
Eric-Arellano May 10, 2019
300e7f1
Get test working by touching __init__.py
Eric-Arellano May 10, 2019
f25c5b9
Try to get the rule working
Eric-Arellano May 10, 2019
2b1816f
Go back to inlining the rule
Eric-Arellano May 10, 2019
ce34286
Generify source root prefix parsing!
Eric-Arellano May 10, 2019
6a5e9cd
Generify __init__.py injection!
Eric-Arellano May 11, 2019
c86e6ca
Remove rule that didn't work out to start cleanup work
Eric-Arellano May 11, 2019
f22ea0f
Pytest stdout will show relative path, although our output will show …
Eric-Arellano May 11, 2019
6de654a
Deduplicate function to find missing __init__.py files
Eric-Arellano May 11, 2019
9459047
Merge branch 'master' of github.com:pantsbuild/pants into v2-pytest-a…
Eric-Arellano May 13, 2019
21c638b
Remove __init__.py from BUILD
Eric-Arellano May 14, 2019
a60cb54
Merge branch 'master' of github.com:pantsbuild/pants into v2-pytest-a…
Eric-Arellano May 14, 2019
cc92bac
Improve comments
Eric-Arellano May 14, 2019
dba4a57
Link comments to newly created TODO issues.
Eric-Arellano May 14, 2019
f8a7d71
Try to make prefix stripping code more self-documenting
Eric-Arellano May 14, 2019
cebcb32
Review feedback
Eric-Arellano May 14, 2019
3c1462f
Fix lint due to Python 2 scoping issue
Eric-Arellano May 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/python/pants/backend/python/rules/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ python_library(
'src/python/pants/engine:rules',
'src/python/pants/engine:selectors',
'src/python/pants/rules/core',
'src/python/pants/source:source',
],
)
50 changes: 44 additions & 6 deletions src/python/pants/backend/python/rules/python_test_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,18 @@

from future.utils import text_type

from pants.backend.python.subsystems.pex_build_util import identify_missing_init_files
from pants.backend.python.subsystems.pytest import PyTest
from pants.backend.python.subsystems.python_setup import PythonSetup
from pants.engine.fs import Digest, MergedDirectories, Snapshot, UrlToFetch
from pants.engine.fs import (Digest, DirectoryWithPrefixToStrip, FilesContent, MergedDirectories,
Snapshot, UrlToFetch)
from pants.engine.isolated_process import (ExecuteProcessRequest, ExecuteProcessResult,
FallibleExecuteProcessResult)
from pants.engine.legacy.graph import TransitiveHydratedTarget
from pants.engine.rules import optionable_rule, rule
from pants.engine.selectors import Get
from pants.rules.core.core_test_model import Status, TestResult
from pants.source.source_root import SourceRootConfig


# This class currently exists so that other rules could be added which turned a HydratedTarget into
Expand All @@ -44,8 +47,10 @@ def parse_interpreter_constraints(python_setup, python_target_adaptors):

# TODO: Support deps
# TODO: Support resources
@rule(PyTestResult, [TransitiveHydratedTarget, PyTest, PythonSetup])
def run_python_test(transitive_hydrated_target, pytest, python_setup):
# TODO(7697): Use a dedicated rule for removing the source root prefix, so that this rule
# does not have to depend on SourceRootConfig.
@rule(PyTestResult, [TransitiveHydratedTarget, PyTest, PythonSetup, SourceRootConfig])
def run_python_test(transitive_hydrated_target, pytest, python_setup, source_root_config):
target_root = transitive_hydrated_target.root

# TODO: Inject versions and digests here through some option, rather than hard-coding it.
Expand Down Expand Up @@ -98,17 +103,49 @@ def run_python_test(transitive_hydrated_target, pytest, python_setup):
requirements_pex_response = yield Get(
ExecuteProcessResult, ExecuteProcessRequest, requirements_pex_request)

# Gather sources.
source_roots = source_root_config.get_source_roots()

# Gather sources and adjust for the source root.
# TODO: make TargetAdaptor return a 'sources' field with an empty snapshot instead of raising to
# simplify the hasattr() checks here!
# TODO(7714): restore the full source name for the stdout of the Pytest run.
all_sources_digests = []
for maybe_source_target in all_targets:
if hasattr(maybe_source_target.adaptor, 'sources'):
sources_snapshot = maybe_source_target.adaptor.sources.snapshot
all_sources_digests.append(sources_snapshot.directory_digest)
sources_source_root = source_roots.find_by_path(maybe_source_target.adaptor.address.spec_path)
digest_adjusted_for_source_root = yield Get(
Digest,
DirectoryWithPrefixToStrip(
directory_digest=sources_snapshot.directory_digest,
prefix=sources_source_root.path
)
)
all_sources_digests.append(digest_adjusted_for_source_root)

sources_digest = yield Get(
Digest, MergedDirectories(directories=tuple(all_sources_digests)),
)

# TODO(7716): add a builtin rule to go from MergedDirectories->Snapshot or Digest->Snapshot.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh! I'm pretty sure we actually can do this now! (There were reasons we couldn't before, but I think they're all gone).

I'd be very happy to talk you through how to do this - it would look very similar to #7699 but instead of calling a new fs::Snapshot::strip_prefix function it would call the existing fs::Snapshot::from_digest function.

Definitely doesn't need to block this PR, but would be good to do soon :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yay! I've always wanted to write Rust, so with a little guidance would be happy to do this.

# TODO(7715): generalize the injection of __init__.py files.
file_contents = yield Get(FilesContent, Digest, sources_digest)
file_paths = [fc.path for fc in file_contents]
injected_inits = tuple(sorted(identify_missing_init_files(file_paths)))
touch_init_request = ExecuteProcessRequest(
argv=("touch",) + injected_inits,
output_files=injected_inits,
description="Inject empty __init__.py into all packages without one already.",
input_files=sources_digest,
env={"PATH": os.environ["PATH"]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really, really like this to be taken out either into its own v2 rule with e.g. a @rule(FallibleExecuteProcessResult, [LocalPATHExecuteProcessRequest]) which wraps a normal ExecuteProcessRequest, does some transformation (setting env={"PATH": os.environ["PATH"]}), then does a yield Get(FallIbleExecuteProcessResult, ExecuteProcessRequest, modified_exe_request). I would like this to be done in this PR if possible, as I would prefer to avoid having references to the local PATH start sprouting unchecked (and I would love to be able to modify just a single @rule in order to modify the behavior of all rules which use anything from the host PATH!).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to do this in a followup, but request not blocking on this here. 3 V2 Pytest PRs are blocked by this one landing, which block any progress on the remoting project.

)

touch_init_result = yield Get(ExecuteProcessResult, ExecuteProcessRequest, touch_init_request)

all_input_digests = all_sources_digests + [
all_input_digests = [
sources_digest,
requirements_pex_response.output_directory_digest,
touch_init_result.output_directory_digest
]
merged_input_files = yield Get(
Digest,
Expand Down Expand Up @@ -138,4 +175,5 @@ def rules():
run_python_test,
optionable_rule(PyTest),
optionable_rule(PythonSetup),
optionable_rule(SourceRootConfig),
]
48 changes: 26 additions & 22 deletions src/python/pants/backend/python/subsystems/pex_build_util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,27 @@ def targets_by_platform(targets, python_setup):
return d


def identify_missing_init_files(sources):
"""Return the list of paths that would need to be added to ensure that every package has
an __init__.py. """
packages = set()
for source in sources:
if source.endswith('.py'):
pkg_dir = os.path.dirname(source)
if pkg_dir and pkg_dir not in packages:
package = ''
for component in pkg_dir.split(os.sep):
package = os.path.join(package, component)
packages.add(package)

missing_init_files = set()
for package in packages:
init_file = os.path.join(package, '__init__.py')
if init_file not in sources:
missing_init_files.add(init_file)
return missing_init_files


def _create_source_dumper(builder, tgt):
if type(tgt) == Files:
# Loose `Files` as opposed to `Resources` or `PythonTarget`s have no (implied) package structure
Expand Down Expand Up @@ -305,31 +326,14 @@ def add_sources_from(self, tgt):
def _prepare_inits(self):
chroot = self._builder.chroot()
sources = chroot.get('source') | chroot.get('resource')

packages = set()
for source in sources:
if source.endswith('.py'):
pkg_dir = os.path.dirname(source)
if pkg_dir and pkg_dir not in packages:
package = ''
for component in pkg_dir.split(os.sep):
package = os.path.join(package, component)
packages.add(package)

missing_pkg_files = []
for package in packages:
pkg_file = os.path.join(package, '__init__.py')
if pkg_file not in sources:
missing_pkg_files.append(pkg_file)

if missing_pkg_files:
missing_init_files = identify_missing_init_files(sources)
if missing_init_files:
with temporary_file() as ns_package:
ns_package.write(b'__import__("pkg_resources").declare_namespace(__name__)')
ns_package.flush()
for missing_pkg_file in missing_pkg_files:
self._builder.add_source(ns_package.name, missing_pkg_file)

return missing_pkg_files
for missing_init_file in missing_init_files:
self._builder.add_source(ns_package.name, missing_init_file)
return missing_init_files

def set_emit_warnings(self, emit_warnings):
self._builder.info.emit_warnings = emit_warnings
Expand Down
15 changes: 10 additions & 5 deletions testprojects/tests/python/pants/dummies/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,20 @@ python_tests(

python_library(
name = 'example_lib',
sources = [
'example_source.py',
'__init__.py',
sources = ['example_source.py'],
)

python_tests(
name = 'target_with_source_dep_absolute_import',
sources = ['test_with_source_dep_absolute_import.py'],
dependencies = [
':example_lib',
],
)

python_tests(
name = 'target_with_source_dep',
sources = ['test_with_source_dep.py'],
name = 'target_with_source_dep_relative_import',
sources = ['test_with_source_dep_relative_import.py'],
dependencies = [
':example_lib',
],
Expand Down
7 changes: 7 additions & 0 deletions testprojects/tests/python/pants/dummies/test_with_source_dep_absolute_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from __future__ import absolute_import

from pants.dummies.example_source import add_two


def test_external_method():
assert add_two(2) == 4
0 ...hon/pants/dummies/test_with_source_dep.py → ...s/test_with_source_dep_relative_import.py
View file
Open in desktop
File renamed without changes.
49 changes: 36 additions & 13 deletions tests/python/pants_test/rules/test_test_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def test_passing_python_test(self):
plugins: SOME_TEXT
collected 1 item

testprojects/tests/python/pants/dummies/test_pass.py . [100%]
pants/dummies/test_pass.py . [100%]

=========================== 1 passed in SOME_TEXT ===========================

Expand All @@ -95,7 +95,7 @@ def test_failing_python_test(self):
plugins: SOME_TEXT
collected 1 item

testprojects/tests/python/pants/dummies/test_fail.py F [100%]
pants/dummies/test_fail.py F [100%]

=================================== FAILURES ===================================
__________________________________ test_fail ___________________________________
Expand All @@ -104,36 +104,59 @@ def test_fail():
> assert False
E assert False

testprojects/tests/python/pants/dummies/test_fail.py:2: AssertionError
pants/dummies/test_fail.py:2: AssertionError
=========================== 1 failed in SOME_TEXT ===========================


testprojects/tests/python/pants/dummies:failing_target ..... FAILURE
"""),
)

def test_source_dep(self):
def test_source_dep_absolute_import(self):
pants_run = self.run_passing_pants_test([
'testprojects/tests/python/pants/dummies:target_with_source_dep',
'testprojects/tests/python/pants/dummies:target_with_source_dep_absolute_import',
])
self.assert_fuzzy_string_match(
pants_run.stdout_data,
dedent("""\
testprojects/tests/python/pants/dummies:target_with_source_dep stdout:
testprojects/tests/python/pants/dummies:target_with_source_dep_absolute_import stdout:
============================= test session starts ==============================
platform SOME_TEXT
rootdir: SOME_TEXT
plugins: SOME_TEXT
collected 1 item

testprojects/tests/python/pants/dummies/test_with_source_dep.py . [100%]
pants/dummies/test_with_source_dep_absolute_import.py . [100%]

=========================== 1 passed in SOME_TEXT ===========================


testprojects/tests/python/pants/dummies:target_with_source_dep ..... SUCCESS
testprojects/tests/python/pants/dummies:target_with_source_dep_absolute_import ..... SUCCESS
""")
)
)

def test_source_dep_relative_import(self):
pants_run = self.run_passing_pants_test([
'testprojects/tests/python/pants/dummies:target_with_source_dep_relative_import',
])
self.assert_fuzzy_string_match(
pants_run.stdout_data,
dedent("""\
testprojects/tests/python/pants/dummies:target_with_source_dep_relative_import stdout:
============================= test session starts ==============================
platform SOME_TEXT
rootdir: SOME_TEXT
plugins: SOME_TEXT
collected 1 item

pants/dummies/test_with_source_dep_relative_import.py . [100%]

=========================== 1 passed in SOME_TEXT ===========================


testprojects/tests/python/pants/dummies:target_with_source_dep_relative_import ..... SUCCESS
""")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer keeping parentheses joined here instead of spreading them over a new line, but I think the style is inconsistent all over the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair. For now, this is using the file convention which I'm going to stick with.

This is precisely why I want to add Black to our codebase - either style works just fine, but the issue is that this statement is unfortunately true :/

I think the style is inconsistent all over the repo.

)

def test_thirdparty_dep(self):
pants_run = self.run_passing_pants_test([
Expand All @@ -149,7 +172,7 @@ def test_thirdparty_dep(self):
plugins: SOME_TEXT
collected 1 item

testprojects/tests/python/pants/dummies/test_with_thirdparty_dep.py . [100%]
pants/dummies/test_with_thirdparty_dep.py . [100%]

=========================== 1 passed in SOME_TEXT ===========================

Expand All @@ -173,7 +196,7 @@ def test_mixed_python_tests(self):
plugins: SOME_TEXT
collected 1 item

testprojects/tests/python/pants/dummies/test_fail.py F [100%]
pants/dummies/test_fail.py F [100%]

=================================== FAILURES ===================================
__________________________________ test_fail ___________________________________
Expand All @@ -182,7 +205,7 @@ def test_fail():
> assert False
E assert False

testprojects/tests/python/pants/dummies/test_fail.py:2: AssertionError
pants/dummies/test_fail.py:2: AssertionError
=========================== 1 failed in SOME_TEXT ===========================

testprojects/tests/python/pants/dummies:passing_target stdout:
Expand All @@ -192,7 +215,7 @@ def test_fail():
plugins: SOME_TEXT
collected 1 item

testprojects/tests/python/pants/dummies/test_pass.py . [100%]
pants/dummies/test_pass.py . [100%]

=========================== 1 passed in SOME_TEXT ===========================

Expand Down