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

Use `./pants.pex`, not `./pants`, to run internal integration tests #8183

Merged

Conversation

@Eric-Arellano
Copy link
Contributor

commented Aug 18, 2019

Changes to get --chroot working

As prework to running integration tests with --chroot and then remoting, we first must make two major changes to how we handle integration tests:

  1. Having integration tests depend on //:build_root. This ensures that get_buildroot() properly resolves to the chrooted folder in .pants.d, rather than traversing all the way up to the original build root.
  2. Using ./pants.pex rather than ./pants in integration tests. Because V1 strips the prefix of source roots, whereby src/python/pants becomes pants, we cannot also have a file named pants in the same build root. Instead, we can safely depend on ./pants.pex.

Autogeneration of pants.pex

Now that we depend on pants.pex, it is very easy for this file to fall out-of-date and to unintentionally be testing old code.

We work around this via the design in #8209. First, we start caching built PEXes in ~/.cache/pants, with the ID derived from all the source files used for Pants. This allows us to quickly switch between branches without having to regenerate the PEX every time.

Then, we teach ./pants to run bootstrap_pants_pex.sh whenever test is one of the goals and it's not CI. This will ensure that the file is always up-to-date.

@Eric-Arellano Eric-Arellano changed the title WIP: Run majority of integration tests with `--chroot` WIP: Set up whitelist for `--chroot` integration tests Aug 18, 2019

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:chroot-integration-tests branch from f763733 to 9be69bb Aug 18, 2019

@Eric-Arellano Eric-Arellano changed the title WIP: Set up whitelist for `--chroot` integration tests Set up whitelist for `--chroot` integration tests Aug 18, 2019

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:chroot-integration-tests branch from 9be69bb to 4d24839 Aug 20, 2019

@Eric-Arellano Eric-Arellano marked this pull request as ready for review Aug 20, 2019

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:chroot-integration-tests branch from 4d24839 to f59a8c2 Aug 20, 2019

@@ -167,6 +167,13 @@ To run tests for a specific target, run the below, substituting the target(s) wi
:::bash
$ ./pants test tests/python/pants_test/util:strutil

Before running integration tests, you must first bootstrap `pants.pex` as below. Note that this

This comment has been minimized.

Copy link
@stuhood

stuhood Aug 20, 2019

Member

So, while I'm fine with requiring the pex to run integration tests (it makes it the cost of integration tests more apparent, yay!), I don't think that it is reasonable to require people to remember to run this.

I think that you'll need to design a solution to either: 1) trigger the pex (re-)build automatically, 2) fail if it hasn't been rebuilt. I think that it is fine for it to be (relatively) hacky, but it's complicated by the fact that this needs to be consumed by both v1 and v2. Given that it needs to support both v1 and v2, a few potential locations to insert the logic:

  1. in the pants script itself
  2. as a side-effect in register.py for a repo-private backend (https://github.com/pantsbuild/pants/tree/master/pants-plugins)
  3. via some other not-yet-existing "load hook" functionality

In all cases there is the additional challenge of identifying that we are running integration tests though.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 20, 2019

Author Contributor

Totally reasonable to ask for this first. Will think more about this this week and hopefully come up with a solution soon. @cosmicexplorer mentioned over Slack the invalidation code we use for re-bootstrapping native_engine.so as potential inspiration.

Thanks for these suggestions!

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 26, 2019

Author Contributor

The scheme to maintain a cache of pants.pex, as described in #8209, is now complete: https://github.com/pantsbuild/pants/pull/8183/files#diff-b5bfb910b73c1c80b6144d21ce1ea199

The main remaining task is to figure out how to automatically call bootstrap_pants_pex.sh. The best I can think of is to check in pants if any of the CLI args start with test? This would mean any test, including any unit tests, would call the script to ensure it has an up-to-date pants.pex. A better scheme would differentiate between integration and unit tests, but I don't know how to do this.

This comment has been minimized.

Copy link
@blorente

blorente Aug 27, 2019

Contributor

This is a wild thought:
Could the OptionsBootstrapper be modified to know what goal(s) it's trying to run? This is the place where we "parse the bare minimum we need of the command line".

That way, we could write a very concise main() function, that would parse the cli, and check the goals. My main goal for this is to make sure that we're avoiding ad-hoc command line parsing, and localize it as much as possible.

Another options is to formalize this functionality under a class, and wrap that in a script. This is related to pantsd, but it has some goal parsing involved.

As for differentiating between unit/integration test, unfortunately --tag is not a bootstrap option, so this solution wouldn't be able to tell them apart.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 27, 2019

Author Contributor

This would probably work. This step happens before the engine collects all BUILD dependencies, right?

However, I want to avoid as much as possible modifying src until we’ve spent more time using this and thinking about the problem.

The way I’m viewing this is that right now we wrote an alpha prototype to unblock remoting integration tests. We will the next few weeks start to understand much better the workflow, and then can formalize this all more in a more informed manner.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 27, 2019

Author Contributor

This would probably work. This step happens before the engine collects all BUILD dependencies, right?

However, I want to avoid as much as possible modifying src until we’ve spent more time using this and thinking about the problem.

The way I’m viewing this is that right now we wrote an alpha prototype to unblock remoting integration tests. We will the next few weeks start to understand much better the workflow, and then can formalize this all more in a more informed manner.

@Eric-Arellano Eric-Arellano changed the title Set up whitelist for `--chroot` integration tests Use `./pants.pex`, not `./pants`, to run integration tests Aug 20, 2019

@blorente
Copy link
Contributor

left a comment

Thanks!

Just some nits and a +1 to Stu's comment :)

tests/python/pants_test/pants_run_integration_test.py Outdated Show resolved Hide resolved
build-support/bin/ci.py Outdated Show resolved Hide resolved
build-support/bin/ci.py Show resolved Hide resolved
build-support/bin/ci.py Outdated Show resolved Hide resolved

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:chroot-integration-tests branch from f59a8c2 to 43a618a Aug 26, 2019

@Eric-Arellano Eric-Arellano changed the title Use `./pants.pex`, not `./pants`, to run integration tests Use `./pants.pex`, not `./pants`, to run internal integration tests Aug 26, 2019

@stuhood
Copy link
Member

left a comment

I think that this will work for now. Thanks!

We should definitely be looking for opportunities to formalize this: one such opportunity might be when we switch to running pants' own tests with v2 by default: once we're using one test runner exclusively, we could think about doing something like having a custom test target type that represents a pants_integration_test, and having a runner for those that builds the pex dynamically, for example.

# a file named `pants`. This is the same reason in
# https://github.com/pantsbuild/pants/pull/8105 we added `BUILD_ROOT` as an alternate way to
# find the build root, because we could not depend on `./pants` for this purpose.
#

This comment has been minimized.

Copy link
@stuhood

stuhood Aug 27, 2019

Member

An annoying issue here is that PantsRunIntegrationTest is a public API. Even more annoying, it's not marked as such: it's in https://pypi.org/project/pantsbuild.pants.testinfra/ though, and is consumed by repos with their own plugin code.

One option would be to capture in both the pants script, and the underlying pex? But then you'd have to figure out how to know to trigger the pex...

Another option might be to subclass this for use by the pants repo itself, and continue to publish the superclass?

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 27, 2019

Author Contributor

One option would be to capture in both the pants script, and the underlying pex? But then you'd have to figure out how to know to trigger the pex...

It's impossible to do this formally, i.e. through BUILD dependencies, because we can't have a folder named pants and a file named pants.

We could maybe informally do this. It looks like if you run the test and pants.pex is not created, which is a dependency now for this file, the worst that will happen beyond a runtime FileNotFoundError is a warning that the glob pants.pex could not be matched.

Another option might be to subclass this for use by the pants repo itself, and continue to publish the superclass?

Hm, I think we do this. Tedious to have to update everywhere, but such is life.

--

Do we roll out a deprecation cycle here? It would be nice to always depend on pants.pex, but we may want to formalize this a bit more before that. Otherwise, Twitter would have to come up with its own bootstrap_pants_pex.sh, for example.

@@ -645,3 +649,18 @@ def do_command_yielding_workdir(self, *args, **kwargs):
else:
self.assert_failure(pants_run)
yield pants_run


class SafePantsRunIntegrationTest(PantsRunIntegrationTest):

This comment has been minimized.

Copy link
@stuhood

stuhood Aug 29, 2019

Member

Oh! So, here's an alternative.

PantsRunIntegrationTest could gain a deprecation warning if pants.pex doesn't exist and uses pants intead, and that suggests either:

  1. subclassing a new UnisolatedPantsRunIntegrationTest which uses pants
  2. overriding the script name to use pants

This would avoid giving the longer name to the behaviour that we're trying to encourage. Sorry... didn't think of it earlier.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:chroot-integration-tests branch 2 times, most recently from 018bf09 to ef489ac Aug 31, 2019

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:chroot-integration-tests branch from ef489ac to a931123 Sep 2, 2019

@illicitonion
Copy link
Contributor

left a comment

Looks good :) Thanks!

build-support/bin/bootstrap_pants_pex.sh Outdated Show resolved Hide resolved
build-support/bin/bootstrap_pants_pex.sh Outdated Show resolved Hide resolved
pants Outdated Show resolved Hide resolved
tests/python/pants_test/pants_run_integration_test.py Outdated Show resolved Hide resolved

@Eric-Arellano Eric-Arellano merged commit 684e043 into pantsbuild:master Sep 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:chroot-integration-tests branch Sep 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.