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

unrevert go test upgrades to test transitively after fixing issues #7326

Merged

Conversation

Projects
None yet
2 participants
@cosmicexplorer
Copy link
Contributor

commented Mar 7, 2019

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!

@cosmicexplorer cosmicexplorer requested review from stuhood and benjyw Mar 7, 2019

@stuhood
Copy link
Member

left a comment

Thanks! I think the breaking change to options will need fixing before landing though.

Show resolved Hide resolved contrib/go/src/python/pants/contrib/go/targets/go_local_source.py Outdated
Show resolved Hide resolved contrib/go/src/python/pants/contrib/go/tasks/go_test.py Outdated
@@ -105,6 +115,8 @@ def run_tests(self, fail_fast, test_targets, args_by_target):
)
go_cmd = self.go_dist.create_go_cmd('test', gopath=gopath, args=cmdline_args)

self.context.log.debug('go_cmd: {}'.format(go_cmd))

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 7, 2019

Member

This should be logged via the workunit I think? Not a problem to leave it though.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 8, 2019

Author Contributor

Does self.context.log.debug() not do that already? Looking into plaintext_reporter.py I'm seeing that _COLOR_BY_LEVEL is cyan for debug logs, which is what this comes out as, and self.context.log ends up being run_tracker.logger (according to context.py), and in run_tracker.py the def log(self) method logs against self._threadlocal.current_workunit, which seems like it might do the right thing?

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 12, 2019

Member

It doesn't get rendered to stdout, but it does get rendered in ./pants server --open, IIRC.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 13, 2019

Author Contributor

I definitely see it in the output when I run pants with -ldebug -- maybe it goes to stderr?

@stuhood
Copy link
Member

left a comment

Thanks!

help='Flags to pass in to `go test` tool.')
# TODO: make a shlexed flags option type!
register('--shlexed-build-and-test-flags', type=list, member_type=str, fingerprint=True,

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 12, 2019

Member

Would it be accurate to just call this just --go-flags? It's already on a test task, and presumably "build" is implicit.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 13, 2019

Author Contributor

From go help test, I'm seeing e.g.:

In addition to the build flags, the flags handled by 'go test' itself are:

And it seems to be pretty specific about which flags are accepted by the binary (specifically for building and testing). I suspect the original option name was trying to be specific about that, but I do not have enough familiarity with go to know. --shlexed-build-and-test-flags is a ridiculous name, but I was thinking we would drop the shlexed part once the original option completes the deprecation cycle.

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 14, 2019

Member

Ok. Let's just remember to do the second deprecation cycle.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 24, 2019

Author Contributor

I've added specific instructions on these sequential deprecation cycles in the removal_hint of the --build-and-test-flags argument!

cosmicexplorer added some commits Mar 6, 2019

Revert "Revert "make GoTest subclass PartitionedTestRunnerTaskMixin t…
…o test transitively" (#7212)"

This reverts commit b8999e1.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-go-test-transitive-patch branch 2 times, most recently from 087bf87 to d6ef9a1 Mar 24, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-go-test-transitive-patch branch from d6ef9a1 to ddd41aa Mar 24, 2019

@cosmicexplorer cosmicexplorer merged commit df28767 into pantsbuild:master Mar 31, 2019

1 check passed

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

cosmicexplorer added a commit that referenced this pull request Apr 12, 2019

fix go test flags bug and add integration test (#7539)
### Problem

#7326 added back the ability to go test transitive deps, but also failed to test adding any options (including the new `--shlexed-build-and-test-flags`) to the command line, which ends up raising an exception.

### Solution

- Correctly flatten `--shlexed-build-and-test-flags` when creating the `go test` command line.
- Add an integration test with the use of options to avoid regressions.

### Result

Go testing doesn't fail when provided extra flags!
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.