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

Stabilize all tests of NailgunTask subclasses. #7866

Merged
merged 2 commits into from Jun 9, 2019

Conversation

Projects
None yet
3 participants
@jsirois
Copy link
Member

commented Jun 6, 2019

Since there is no need to re-test nailgun behavior for each
NailgunTask subclass, turn off nailgunning in unit tests. Previously
we did this piecemeal and left out the JarTask test for one. This led
to problems in CI under the v2 runner's highly parallel regime.

Fixes #7865

@jsirois

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Local runs of affected targets showed a ~15% slowdown. Roughly 2 seconds -> 2.3 seconds. I, for one, am happy to accept this trade-off.

@Eric-Arellano
Copy link
Contributor

left a comment

Great. This looks very sensible to me.

My only feedback is to take a pass over the title and PR description. For example, title could be "Stabilize all tests of Nailgun subclasses by changing execution strategy". For the performance change that you mentioned in your comment, it would be helpful to include in the PR description. Reason I'm suggesting this is it took me until reaching nailgun_task_test_base.py to really understand what the fix is here.

Thank you for taking the time to fix this!

@stuhood

stuhood approved these changes Jun 7, 2019

Copy link
Member

left a comment

Thank you @jsirois!

super(NailgunTaskTestBase, self).tearDown()
# Kill any nailguns launched in our ephemeral build root.
NailgunProcessGroup(metadata_base_dir=self.subprocess_dir).killall()
self.set_options(execution_strategy=NailgunTask.ExecutionStrategy.subprocess)

This comment has been minimized.

Copy link
@stuhood

stuhood Jun 7, 2019

Member

Would it be ... reasonable to set this on TaskBase instead?

This comment has been minimized.

Copy link
@jsirois

jsirois Jun 7, 2019

Author Member

No. If we can't count on writers to subclass the right test baseclass for the corresponding task or reviewers to catch this all hope is lost.

@jsirois

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

The failures here reveal a bunch of wonkiness / sloppiness in semantics with how we compute classpaths. Working on fixes for the subprocess execution case which does not have the bedrock of the cwd=buildroot that the nailgun case has.

jsirois added a commit to jsirois/pants that referenced this pull request Jun 8, 2019

NailgunTasks execute java with consistent cwd.
Previously, nailgun-mode executions would run with the cwd set to the
buildroot but subprocess mode executions would inherit cwd. These
usually aligned since pants is run from the buildroot, but could
misalign in tests where a test buildroot is propped up.

Ensure cwd is always explicitly the buildroot for NailgunTasks and fix
executor cwd plumbing to effect this with a test added that proves this
all works.

Additionally, fix ScalaPlatform classpaths, which had been relativized
to the buildroot to support snapshotting. Maintain the snapshot relative
paths, but leave the in-memory classpath un-altered and populated with
absolute paths like all other runtime classpaths.

Prep work for pantsbuild#7866.
@jsirois

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

My only feedback is to take a pass over the title and PR description.

One unwritten rule I follow in all my commits is to match linux / git commit message norms: https://git-scm.com/docs/git-commit#_discussion - In particular, 50 character max subjects, 72 character max message body lines.

I left the title as-is as a result (47 chars as-is) but moved the last body sentence to be the first since it already explains the the fix was to change the execution strategy - in particular "turn off nailgunning in unit tests".

@Eric-Arellano
Copy link
Contributor

left a comment

Thanks John! Description looks good now.

jsirois added a commit that referenced this pull request Jun 9, 2019

NailgunTasks execute java with consistent cwd. (#7872)
Previously, nailgun-mode executions would run with the cwd set to the
buildroot but subprocess mode executions would inherit cwd. These
usually aligned since pants is run from the buildroot, but could
misalign in tests where a test buildroot is propped up.

Ensure cwd is always explicitly the buildroot for NailgunTasks and fix
executor cwd plumbing to effect this with a test added that proves this
all works.

Additionally, fix ScalaPlatform classpaths, which had been relativized
to the buildroot to support snapshotting. Maintain the snapshot relative
paths, but leave the in-memory classpath un-altered and populated with
absolute paths like all other runtime classpaths.

Prep work for #7866.

jsirois added some commits Jun 6, 2019

Stabilize all tests of NailgunTask subclasses.
Previously we did this piecemeal and left out the `JarTask` test for
one. This led to problems in CI under the v2 runner's highly parallel
regime. Since there is no need to re-test nailgun behavior for each
`NailgunTask` subclass, turn off nailgunning in unit tests.

Fixes #7865

@jsirois jsirois force-pushed the jsirois:issues/7865 branch from 6953690 to fcbaac2 Jun 9, 2019

@jsirois jsirois merged commit bb2a1ff into pantsbuild:master Jun 9, 2019

1 check passed

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

@jsirois jsirois deleted the jsirois:issues/7865 branch Jun 9, 2019

cattibrie added a commit to cattibrie/pants that referenced this pull request Jun 19, 2019

NailgunTasks execute java with consistent cwd. (pantsbuild#7872)
Previously, nailgun-mode executions would run with the cwd set to the
buildroot but subprocess mode executions would inherit cwd. These
usually aligned since pants is run from the buildroot, but could
misalign in tests where a test buildroot is propped up.

Ensure cwd is always explicitly the buildroot for NailgunTasks and fix
executor cwd plumbing to effect this with a test added that proves this
all works.

Additionally, fix ScalaPlatform classpaths, which had been relativized
to the buildroot to support snapshotting. Maintain the snapshot relative
paths, but leave the in-memory classpath un-altered and populated with
absolute paths like all other runtime classpaths.

Prep work for pantsbuild#7866.

cattibrie added a commit to cattibrie/pants that referenced this pull request Jun 19, 2019

Stabilize all tests of NailgunTask subclasses. (pantsbuild#7866)
Previously we did this piecemeal and left out the `JarTask` test for
one. This led to problems in CI under the v2 runner's highly parallel
regime. Since there is no need to re-test nailgun behavior for each
`NailgunTask` subclass, turn off nailgunning in unit tests.

Fixes pantsbuild#7865
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.