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.

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

Properly strip source root prefixes for V2 Pytest runner #8185

Merged

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Aug 19, 2019

Problem

#7696 introduced support for source roots to V2 Pytest for the first time, but it did not support loose files. #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: Properly handle source roots and resources with V2 Pytest runner #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:

def get_chroot_path(relpath: str) -> str:
if type(tgt) == Files:
# Loose `Files`, as opposed to `Resources` or `PythonTarget`s, have no (implied) package
# structure and so we chroot them relative to the build root so that they can be accessed
# via the normal Python filesystem APIs just as they would be accessed outside the
# chrooted environment. NB: This requires we mark the pex as not zip safe so
# these `Files` can still be accessed in the context of a built pex distribution.
builder.info.zip_safe = False
return relpath
return str(Path(relpath).relative_to(tgt.target_base))

Result

V2 Pytest now supports both source roots and loose files! This allows us to run most contrib unit tests with V2 and unblocks #8113.

Because over 95% of unit tests are now remoted, we go back to only one CI shard for unit tests.

Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Eric-Arellano Eric-Arellano force-pushed the actually-handle-source-roots branch 2 times, most recently from 3725188 to a42ed86 Compare August 20, 2019 01:28
@stuhood stuhood force-pushed the actually-handle-source-roots branch from 8d1e46b to ce918da Compare August 20, 2019 16:11
@stuhood
Copy link
Sponsor Member

stuhood commented Aug 20, 2019

Your bootstrap shards raced another PR landing. Have rebased.

@stuhood
Copy link
Sponsor Member

stuhood commented Aug 20, 2019

There appear to be some actual failures in CI.

@Eric-Arellano
Copy link
Contributor Author

b464557 fixes the unit test failure. Test discovery was failing for test_coursier_resolve because the side-effects of importing psutil.tests caused unittest.TestCase to have __init__() defined, meaning that Pytest would no longer discover the targets.

Very unclear why this was not happening with V1 test runner or V2 before this change..not going to spend more time trying to answer that, though.

@Eric-Arellano Eric-Arellano merged commit 538b976 into pantsbuild:master Aug 25, 2019
@Eric-Arellano Eric-Arellano deleted the actually-handle-source-roots branch August 25, 2019 05:54
jsirois pushed a commit that referenced this pull request Sep 25, 2019
Problem
-------
#8185 introduced proper support stripping source roots for targets, but keeping the full path for loose files.

Solution
--------
Now that we are porting the rest of the V2 pipeline, e.g. #8236, this functionality should be extracted out into a new rule.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants