New issue
Advanced search Search tips

Issue 817688 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 820292
issue 835991



Sign in to add a comment

Tast: test runners and test bundles should read args from stdin instead of parsing command-line flags

Project Member Reported by derat@chromium.org, Mar 1 2018

Issue description

It's common to run a newer version of the tast command against an older version of local_test_runner or vice versa: in the lab, the tast command (and remote_test_runner) are deployed via pushes to prod, while local_test_runner is baked into DUT system images. The tast command passes command line flags to test runners to tell them how to behave, and this makes it difficult to add or remove functionality -- if the runners see a flag that they don't recognize, they abort. Long command lines are also unwieldy.

I'm updating things so that the tast command will JSON-marshal a new Args struct to test runners' stdin (by default, Go's json.Decoder happily skips fields that aren't defined in a destination struct). I'll continue to support a small set of command-line flags so that test runners can be executed manually.

For consistency, I'll probably give runner-to-bundle communication a similar treatment, although the same versioning concerns don't really apply there.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/8bfdf07d706f14ec38a1f9b40e473a01aa31b734

commit 8bfdf07d706f14ec38a1f9b40e473a01aa31b734
Author: Daniel Erat <derat@chromium.org>
Date: Fri Mar 02 00:36:29 2018

tast: Update test runners to accept args over stdin.

Update local_test_runner and remote_test_runner to read a
new Args struct from stdin when no command-line arguments
are supplied. The tast command and local_test_runner don't
get updated in lockstep, and this is intended to make it
easier to add or remove arguments in the future without
breaking old binaries.

A future change will update the tast command to write Args
structs rather than use command-line flags.

BUG= chromium:817688 
TEST=updated unit tests; also verified that "run" and "list"
     subcommands work with both -type=local and -type=remote
     when running the not-yet-updated tast command against
     updated test runners

Change-Id: I6877a54ad14931ccdfeb5b72a61fb1cf224ae92d
Reviewed-on: https://chromium-review.googlesource.com/942682
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/8bfdf07d706f14ec38a1f9b40e473a01aa31b734/src/chromiumos/tast/runner/runner_test.go
[modify] https://crrev.com/8bfdf07d706f14ec38a1f9b40e473a01aa31b734/src/chromiumos/cmd/local_test_runner/main.go
[modify] https://crrev.com/8bfdf07d706f14ec38a1f9b40e473a01aa31b734/src/chromiumos/tast/runner/runner.go
[modify] https://crrev.com/8bfdf07d706f14ec38a1f9b40e473a01aa31b734/src/chromiumos/cmd/remote_test_runner/main.go

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 6 2018

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/b0b3c8c54d244ffa737c4e8400e1ff3ae1db1d58

commit b0b3c8c54d244ffa737c4e8400e1ff3ae1db1d58
Author: Daniel Erat <derat@chromium.org>
Date: Thu Mar 08 08:37:43 2018

tast: Clean up test data and test-printing code.

Make the tast command just get the listing of necessary data
files from the test listing supplied by the runner. This
removes the need for a dedicated "list data files" command.

BUG= chromium:817688 
TEST=unit tests pass; also verified that the
     example.DataFiles test passes and that the "list"
     command still works

Change-Id: I4d65ebbc27d4e83cfcf3ebdd2b887e3e0845d17d
Reviewed-on: https://chromium-review.googlesource.com/954287
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/b0b3c8c54d244ffa737c4e8400e1ff3ae1db1d58/src/chromiumos/cmd/tast/run/print.go
[modify] https://crrev.com/b0b3c8c54d244ffa737c4e8400e1ff3ae1db1d58/src/chromiumos/cmd/tast/run/line.go
[modify] https://crrev.com/b0b3c8c54d244ffa737c4e8400e1ff3ae1db1d58/src/chromiumos/tast/runner/runner.go
[modify] https://crrev.com/b0b3c8c54d244ffa737c4e8400e1ff3ae1db1d58/src/chromiumos/cmd/tast/run/local.go
[modify] https://crrev.com/b0b3c8c54d244ffa737c4e8400e1ff3ae1db1d58/src/chromiumos/cmd/tast/run/remote.go

Comment 4 by derat@chromium.org, Mar 8 2018

Blocking: 820292
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/e588107e314bc20d63a2b7a66094e5d468984493

commit e588107e314bc20d63a2b7a66094e5d468984493
Author: Daniel Erat <derat@chromium.org>
Date: Thu Mar 15 16:04:08 2018

tast: Remove old runner flags and list-data-files command.

Remove the -report, -listdata, and -listtests flags from
local_test_runner and remote_test_runner. The tast command
always passes args to runners via stdin now.

Also remove ListDataMode and its associated code from the
runner package. The tast command uses RunTestsMode to get
the set of necessary data files now.

BUG= chromium:817688 
TEST="tast run" works for both local and remote tests

Change-Id: Iac659293a85dca74811d3275b8754fde7f6c7c2e
Reviewed-on: https://chromium-review.googlesource.com/963020
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/e588107e314bc20d63a2b7a66094e5d468984493/src/chromiumos/tast/runner/doc.go
[delete] https://crrev.com/b0b3c8c54d244ffa737c4e8400e1ff3ae1db1d58/src/chromiumos/tast/runner/data_files_test.go
[modify] https://crrev.com/e588107e314bc20d63a2b7a66094e5d468984493/src/chromiumos/tast/runner/runner.go
[delete] https://crrev.com/b0b3c8c54d244ffa737c4e8400e1ff3ae1db1d58/src/chromiumos/tast/runner/data_files.go

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/ad093d2ef53d566d7bdfffbfae78f5c32f5f3238

commit ad093d2ef53d566d7bdfffbfae78f5c32f5f3238
Author: Daniel Erat <derat@chromium.org>
Date: Fri Apr 13 08:43:29 2018

tast: Only write control messages when running tests.

When the tast command instructs a test runner to run tests,
the results (including fatal errors) are returned via
JSON-encoded control messages streamed to stdout.

When a test runner is performing some other task like
listing tests, the result (e.g. test data) is still written
to stdout, but the runner is supposed to communicate fatal
errors by exiting with a nonzero status and writing a
message to stderr.

I think I broke this with recent refactoring and made
runners always write errors via control messages regardless
of what they're doing. This results in the tast command
printing useless errors like 'json: cannot unmarshal object
into Go value of type []testing.Test' (after trying to
decode a control message as something else) instead of
more-helpful errors like 'No test bundles matched by
"/usr/local/libexec/tast/bundles/*"'.

BUG= chromium:817688 
TEST=manually verified that useful errors are printed again
     by the "tast list" command and added a unit test

Change-Id: I51aa40479db70ea910cc4c72d49970118531b99d
Reviewed-on: https://chromium-review.googlesource.com/1011708
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/ad093d2ef53d566d7bdfffbfae78f5c32f5f3238/src/chromiumos/tast/runner/runner_test.go
[modify] https://crrev.com/ad093d2ef53d566d7bdfffbfae78f5c32f5f3238/src/chromiumos/tast/runner/runner.go

Comment 7 by derat@chromium.org, Apr 23 2018

Summary: Tast: test runners and test bundles should read args from stdin instead of parsing command-line flags (was: Make tast command write args to test runners' stdin instead of using command-line flags)
I'm going to use this to track updating test bundles as well.

Comment 8 by derat@chromium.org, Apr 23 2018

Blocking: 835991
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/a2067994ce45f7ef3bb69b49b6ba1bbb1541e72a

commit a2067994ce45f7ef3bb69b49b6ba1bbb1541e72a
Author: Daniel Erat <derat@chromium.org>
Date: Tue Apr 24 13:22:11 2018

tast-tests: Update bundle.Local/Remote calls.

Update the "cros" local and remote test bundles' calls to
the bundle.Local and Remote functions to pass stdin and
stdout rather than command-line arguments.

BUG= chromium:817688 
TEST=none
CQ-DEPEND=I1b603d1fa3d624b328deaee2e6f372c92d42f950

Change-Id: I27ff52aabc9e50a8388ce21e189a870e019b962d
Reviewed-on: https://chromium-review.googlesource.com/1025111
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/a2067994ce45f7ef3bb69b49b6ba1bbb1541e72a/src/chromiumos/tast/local/bundles/cros/main.go
[modify] https://crrev.com/a2067994ce45f7ef3bb69b49b6ba1bbb1541e72a/src/chromiumos/tast/remote/bundles/cros/main.go

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/95deec70f0b60f1341ae2b772cfd7f59437c1257

commit 95deec70f0b60f1341ae2b772cfd7f59437c1257
Author: Daniel Erat <derat@chromium.org>
Date: Tue Apr 24 13:22:12 2018

tast: Make test bundles read args from stdin.

Update the chromiumos/tast/bundle package to read
JSON-marshaled arguments from stdin rather than parsing
command-line flags.

This is similar to the treatment that the runner package got
in 8bfdf07d706f, but the bundle package doesn't continue to
support a subset of command-line flags so that bundles can
be run manually. Rather, the code for handling manual test
runs is moved into the runner package -- bundles always read
args from stdin and write control messages to stdout, and
test runners translate those control messages to
human-readable logging when run manually rather than being
executed by the tast command.

BUG= chromium:817688 
TEST=updated unit tests; also manually verified that local
     and remote tests still work when run manually or via
     the tast command
CQ-DEPEND=I19cdbba8848048fb57a00d913ca6c797b4822982
CQ-DEPEND=I27ff52aabc9e50a8388ce21e189a870e019b962d

Change-Id: I1b603d1fa3d624b328deaee2e6f372c92d42f950
Reviewed-on: https://chromium-review.googlesource.com/1024667
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/95deec70f0b60f1341ae2b772cfd7f59437c1257/src/chromiumos/tast/bundle/local_test.go
[modify] https://crrev.com/95deec70f0b60f1341ae2b772cfd7f59437c1257/src/chromiumos/tast/bundle/doc.go
[modify] https://crrev.com/95deec70f0b60f1341ae2b772cfd7f59437c1257/src/chromiumos/tast/bundle/bundle.go
[modify] https://crrev.com/95deec70f0b60f1341ae2b772cfd7f59437c1257/src/chromiumos/tast/bundle/local.go
[modify] https://crrev.com/95deec70f0b60f1341ae2b772cfd7f59437c1257/src/chromiumos/tast/bundle/remote.go
[modify] https://crrev.com/95deec70f0b60f1341ae2b772cfd7f59437c1257/src/chromiumos/tast/bundle/remote_test.go
[modify] https://crrev.com/95deec70f0b60f1341ae2b772cfd7f59437c1257/src/chromiumos/tast/bundle/bundle_test.go
[add] https://crrev.com/95deec70f0b60f1341ae2b772cfd7f59437c1257/src/chromiumos/tast/bundle/args.go

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/33a8ffdae88b38db45ecb6ca41f9ef6f9f926a8f

commit 33a8ffdae88b38db45ecb6ca41f9ef6f9f926a8f
Author: Daniel Erat <derat@chromium.org>
Date: Tue Apr 24 13:22:13 2018

tast: Make test runners pass args to bundles via stdin.

Update the chromiumos/tast/runner package to pass
JSON-marshaled arguments to test bundles via stdin rather
than via command-line flags.

This change also make test runners responsible for logging
test output when the runners are run manually rather than
via the tast command. Formerly, test bundles were
responsible for writing human-readable log messages rather
than control messages in this case.

BUG= chromium:817688 
TEST=updated unit tests; also manually verified that local
     and remote tests still work when run manually or via
     the tast command
CQ-DEPEND=I1b603d1fa3d624b328deaee2e6f372c92d42f950

Change-Id: I19cdbba8848048fb57a00d913ca6c797b4822982
Reviewed-on: https://chromium-review.googlesource.com/1025110
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/33a8ffdae88b38db45ecb6ca41f9ef6f9f926a8f/src/chromiumos/tast/runner/runner_test.go
[modify] https://crrev.com/33a8ffdae88b38db45ecb6ca41f9ef6f9f926a8f/src/chromiumos/tast/runner/runner.go
[modify] https://crrev.com/33a8ffdae88b38db45ecb6ca41f9ef6f9f926a8f/src/chromiumos/cmd/remote_test_runner/main.go

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/b2540bdb1621b71c8b14d6a593640d5c1f5f8859

commit b2540bdb1621b71c8b14d6a593640d5c1f5f8859
Author: Daniel Erat <derat@chromium.org>
Date: Thu Apr 26 06:00:10 2018

tast-tests: Update bundles to pass stderr.

Make the local and remote "cros" bundles pass os.Stderr to
the bundle.Local and Remote functions.

BUG= chromium:817688 
TEST=built them
CQ-DEPEND=Iea5ffedb7f9d3917d0692110329a7340ceed272f

Change-Id: I8d29b42741b2e2aa5d654cf1b45ebd6b12fb80e9
Reviewed-on: https://chromium-review.googlesource.com/1027313
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/b2540bdb1621b71c8b14d6a593640d5c1f5f8859/src/chromiumos/tast/local/bundles/cros/main.go
[modify] https://crrev.com/b2540bdb1621b71c8b14d6a593640d5c1f5f8859/src/chromiumos/tast/remote/bundles/cros/main.go

Project Member

Comment 14 by bugdroid1@chromium.org, Apr 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/171e3f4d710953de412e2d9da6a81b64b494c984

commit 171e3f4d710953de412e2d9da6a81b64b494c984
Author: Daniel Erat <derat@chromium.org>
Date: Thu Apr 26 06:00:11 2018

tast: Rework bundle execution flow.

Update the bundle package to have a single run() function
that both parses args from stdin and performs the requested
action. This removes some trickiness from readArgs(), which
formerly either listed tests itself or returned a runConfig
struct that callers were expected to pass to runTests().

BUG= chromium:817688 
TEST=unit tests pass; also manually verified that listing
     and running local and remote tests works
CQ-DEPEND=I8d29b42741b2e2aa5d654cf1b45ebd6b12fb80e9

Change-Id: Iea5ffedb7f9d3917d0692110329a7340ceed272f
Reviewed-on: https://chromium-review.googlesource.com/1026944
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/171e3f4d710953de412e2d9da6a81b64b494c984/src/chromiumos/tast/bundle/local_test.go
[modify] https://crrev.com/171e3f4d710953de412e2d9da6a81b64b494c984/src/chromiumos/tast/bundle/doc.go
[modify] https://crrev.com/171e3f4d710953de412e2d9da6a81b64b494c984/src/chromiumos/tast/runner/runner_test.go
[modify] https://crrev.com/171e3f4d710953de412e2d9da6a81b64b494c984/src/chromiumos/tast/bundle/bundle.go
[modify] https://crrev.com/171e3f4d710953de412e2d9da6a81b64b494c984/src/chromiumos/tast/bundle/local.go
[modify] https://crrev.com/171e3f4d710953de412e2d9da6a81b64b494c984/src/chromiumos/tast/bundle/remote.go
[modify] https://crrev.com/171e3f4d710953de412e2d9da6a81b64b494c984/src/chromiumos/tast/bundle/remote_test.go
[modify] https://crrev.com/171e3f4d710953de412e2d9da6a81b64b494c984/src/chromiumos/tast/bundle/args_test.go
[modify] https://crrev.com/171e3f4d710953de412e2d9da6a81b64b494c984/src/chromiumos/tast/bundle/bundle_test.go
[modify] https://crrev.com/171e3f4d710953de412e2d9da6a81b64b494c984/src/chromiumos/tast/bundle/args.go

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/4bd93c19657de7cb92c8079b0a8c15058b07f07b

commit 4bd93c19657de7cb92c8079b0a8c15058b07f07b
Author: Daniel Erat <derat@chromium.org>
Date: Fri Apr 27 16:29:51 2018

tast: Add command package containing StatusError.

Move the bundle package's bundleError type to a new
"command" package and rename it to StatusError so it can be
shared with the runner package.

BUG= chromium:817688 
TEST=unit tests pass

Change-Id: I8f7ab3f23edfdc2ffc953398907e416f8e0cc46b
Reviewed-on: https://chromium-review.googlesource.com/1029421
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/4bd93c19657de7cb92c8079b0a8c15058b07f07b/src/chromiumos/tast/bundle/bundle_test.go
[add] https://crrev.com/4bd93c19657de7cb92c8079b0a8c15058b07f07b/src/chromiumos/tast/command/error_test.go
[modify] https://crrev.com/4bd93c19657de7cb92c8079b0a8c15058b07f07b/src/chromiumos/tast/bundle/bundle.go
[add] https://crrev.com/4bd93c19657de7cb92c8079b0a8c15058b07f07b/src/chromiumos/tast/command/error.go
[modify] https://crrev.com/4bd93c19657de7cb92c8079b0a8c15058b07f07b/src/chromiumos/tast/bundle/args.go

Project Member

Comment 16 by bugdroid1@chromium.org, Apr 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/2a26f12e9a9613a3f2676cad58210bbaa3e3fb2f

commit 2a26f12e9a9613a3f2676cad58210bbaa3e3fb2f
Author: Daniel Erat <derat@chromium.org>
Date: Fri Apr 27 16:29:51 2018

tast: Rework runner execution flow.

Update the runner package to expose a single Run method that
both reads arguments and performs the requested action. This
is similar to 171e3f4d7109's treatment of the bundle
package, but a bit trickier since test runners need to
handle both reporting results via control messages (after
being executed by the tast command) and writing
human-readable output (after being executed manually).

BUG= chromium:817688 
TEST=unit tests pass; also manually verified that listing
     local/remote tests still works, as well as running both
     types both manually and via the tast command

Change-Id: I92f431a6866a35fb00687508441fc9c2f25bc993
Reviewed-on: https://chromium-review.googlesource.com/1031785
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/2a26f12e9a9613a3f2676cad58210bbaa3e3fb2f/src/chromiumos/tast/runner/runner_test.go
[modify] https://crrev.com/2a26f12e9a9613a3f2676cad58210bbaa3e3fb2f/src/chromiumos/tast/runner/sys_info.go
[modify] https://crrev.com/2a26f12e9a9613a3f2676cad58210bbaa3e3fb2f/src/chromiumos/tast/runner/runner.go
[modify] https://crrev.com/2a26f12e9a9613a3f2676cad58210bbaa3e3fb2f/src/chromiumos/cmd/remote_test_runner/main.go
[modify] https://crrev.com/2a26f12e9a9613a3f2676cad58210bbaa3e3fb2f/src/chromiumos/tast/runner/args.go
[modify] https://crrev.com/2a26f12e9a9613a3f2676cad58210bbaa3e3fb2f/src/chromiumos/cmd/local_test_runner/main.go
[add] https://crrev.com/2a26f12e9a9613a3f2676cad58210bbaa3e3fb2f/src/chromiumos/tast/runner/bundles.go

Project Member

Comment 17 by bugdroid1@chromium.org, Apr 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/70b1f4801b221d37e9b33842497d8c35d8c3a14f

commit 70b1f4801b221d37e9b33842497d8c35d8c3a14f
Author: Daniel Erat <derat@chromium.org>
Date: Sat Apr 28 04:27:36 2018

tast: Move some functions in the runner package.

Move a few functions from the runner package into
bundles.go. No functional changes.

BUG= chromium:817688 
TEST=unit tests still pass

Change-Id: Ie14febabfaad310b993b6326691e468025900114
Reviewed-on: https://chromium-review.googlesource.com/1033813
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/70b1f4801b221d37e9b33842497d8c35d8c3a14f/src/chromiumos/tast/runner/args.go
[modify] https://crrev.com/70b1f4801b221d37e9b33842497d8c35d8c3a14f/src/chromiumos/tast/runner/runner.go
[modify] https://crrev.com/70b1f4801b221d37e9b33842497d8c35d8c3a14f/src/chromiumos/tast/runner/bundles.go

Comment 18 by derat@chromium.org, Apr 28 2018

Status: Fixed (was: Started)

Sign in to add a comment