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

make GoTest subclass PartitionedTestRunnerTaskMixin to test transitively #7145

Conversation

cosmicexplorer
Copy link
Contributor

Problem

Resolves #6935.

Solution

  • Extract some logic around chrooting from JUnitRun into PartitionedTestRunnerTaskMixin.
  • Make GoTest mix in PartitionedTestRunnerTaskMixin and implement all of the @abstractmethods (everything worked first try, which makes me very suspicious as well as grateful).
  • Add an integration test for go testing dependent targets.

Result

./pants test.go now has some funky features like --chroot or --no-fast, and tests dependent targets by default instead of requiring this to be done manually.

shutil.copy(src, dest)

@contextmanager
def _chroot(self, targets, workdir):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're now calling this in a subclass, it should lose the underscore prefix. We reserve Python's faux-private convention for symbols used only in the same class/file. The idea being that you shouldn't have to look outside the files for uses when making changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok -- that sounds like we should also lose the underscore on _spawn_and_wait() then?

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Jan 25, 2019

Choose a reason for hiding this comment

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

Did both in 5e9b63c!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I wouldn't have asked you to fix _spawn_and_wait, as that's a preexisting condition... But thanks for doing it anyway.

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 -- is it better to isolate this change to _chroot then (just as a matter of what to do in these cases)? I'm fine with that!

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Very nice! LGTM mod the requested method private->public change.

@cosmicexplorer cosmicexplorer merged commit 7555976 into pantsbuild:master Jan 25, 2019
stuhood added a commit to twitter/pants that referenced this pull request Feb 4, 2019
stuhood pushed a commit that referenced this pull request Feb 4, 2019
stuhood pushed a commit that referenced this pull request Feb 5, 2019
cosmicexplorer added a commit that referenced this pull request Mar 31, 2019
…7326)

### Problem

Resolves #7188. #7145 allowed go tests to test their transitive dependencies (necessary because go targets contain both sources and test files), but it was broken in a few ways, leading to the revert in #7212.

### Solution

- Return an iterable (`[]`) from `collect_files()` so the background cache insertion doesn't fail.
- Make `PartitionedTestRunnerTaskMixin` no-op when there are no invalid targets instead of first calling `run_tests()`.
- Turn `--build-and-test-flags` into a list of shlexed strings instead of a single string.

### Result

The go testing from #7145 should have all its issues fixed!
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.

2 participants