New issue
Advanced search Search tips

Issue 831849 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

tast_Runner should report tests that aren't run

Project Member Reported by derat@chromium.org, Apr 11 2018

Issue description

On https://crrev.com/c/997014, Ilja suggested making the tast_Runner Autotest server test report tests as failed if the tast command fails to write results for them. This is a good idea, and probably straightforward to do by running "tast list" initially to get the list of tests that should be run.

Before doing this, I should rework the "list" command to support operating over both local and remote tests, similar to the change to the "run" command tracked in  issue 793124 .
 

Comment 1 by ihf@chromium.org, Apr 11 2018

One more suggestion. It is a good idea to run tests in alphabetical order (and get this list in such order).
a) It is always the same order (reproducibility).
b) Tests are usually displayed in the dashboards in this order.
c) It makes it easier to figure out which test (first to fail) caused all others after it to fail.

Comment 2 by derat@chromium.org, Apr 11 2018

Re running in alphabetical order, I don't disagree in principle, but there are several factors that prevent that from being feasible across the full list of tests:

- Local (on-DUT) tests are run before remote (on-shard) tests. This is unlikely to change; doing it this way minimizes round trips over the network and simplifies communication between the main tast process and other processes.

- All tests within each bundle are run together before moving on to the next bundle. Right now, there are only local and remote "cros" bundles, but that may change later (if someone wants to add non-public tests, for instance).

Tests happen to run as alphabetically as possible right now given the above constraints, but I should probably add some sorting (both when enumerating bundles and when enumerating tests) to make this explicit.

Comment 3 by ihf@chromium.org, Apr 11 2018

I understand. I hoped to make this suggestion early enough to not cause trouble.

I think tast uses CamelCase. Tradefed does so too, but it is able to do so in a way to add a prefix to names to refer to bundles, which in the end makes everything nicely sorted. Maybe you can use prefix to keep sorting order (TastLocalTest1, TastLocalTest2, TastRemoteTest1, ...)?

Whatever you do, please keep orders deterministic for a build (and ideally across builds).
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 12 2018

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

commit fdb985fd6da7eb5babb5f2cf2f1e38a04c94c862
Author: Daniel Erat <derat@chromium.org>
Date: Thu Apr 12 17:01:00 2018

tast: Sort tests by bundle and by test name.

Update test bundles to always run tests in lexicographical
order, and update test runners to always exec test bundles
in lexicographical order.

I think that typically this happened already, but it was
incidental due to the order in which we compile test files
and the implementation of filepath.Glob.

Local test bundles are still all executed before remote test
bundles, and tests from different bundles are not
interleaved.

BUG= chromium:831849 
TEST=added unit tests; also verified that tests are still
     run in order

Change-Id: Ib2557d71b93cd695d60ef9b98f66fec2edffc521
Reviewed-on: https://chromium-review.googlesource.com/1008615
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Ilja H. Friedel <ihf@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/fdb985fd6da7eb5babb5f2cf2f1e38a04c94c862/src/chromiumos/tast/runner/runner_test.go
[modify] https://crrev.com/fdb985fd6da7eb5babb5f2cf2f1e38a04c94c862/src/chromiumos/tast/bundle/bundle_test.go
[modify] https://crrev.com/fdb985fd6da7eb5babb5f2cf2f1e38a04c94c862/src/chromiumos/tast/runner/runner.go
[modify] https://crrev.com/fdb985fd6da7eb5babb5f2cf2f1e38a04c94c862/src/chromiumos/tast/bundle/bundle.go

Project Member

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

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

commit aae63f6ce449a61f8bda9dae9276080423cfc876
Author: Daniel Erat <derat@chromium.org>
Date: Sat Apr 14 04:37:21 2018

tast: Refactor test-listing code.

Move test-printing functionality out of the cmd/tast/run
package and into listCmd in cmd/tast. cmd/tast/run now takes
a Mode enum that specifies whether it should run or list
tests, and ListTestsMode returns tests via a slice of
TestResult structs, the same as RunTestsMode.

This will make it possible to update the list command to
merge both local and remote tests instead of only listing
one type or the other.

BUG= chromium:831849 
TEST=updated unit tests; also verified that "tast run" and
     "tast list" work as before

Change-Id: I7c1a92ef34cc8c4825c7aa00b663e713e2a380fa
Reviewed-on: https://chromium-review.googlesource.com/1011147
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/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/run_cmd_test.go
[modify] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/run/remote.go
[modify] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/main.go
[delete] https://crrev.com/ad093d2ef53d566d7bdfffbfae78f5c32f5f3238/src/chromiumos/cmd/tast/run/print.go
[add] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/run_wrapper.go
[modify] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/run_cmd.go
[modify] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/run/local.go
[modify] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/run/local_test.go
[modify] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/list_cmd.go
[modify] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/run/results.go
[modify] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/run/remote_test.go
[modify] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/run/config.go
[add] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/run_wrapper_test.go
[add] https://crrev.com/aae63f6ce449a61f8bda9dae9276080423cfc876/src/chromiumos/cmd/tast/list_cmd_test.go

Project Member

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

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

commit eb4d352248edb84c6aed3bf2ca133b0c7e4c51c5
Author: Daniel Erat <derat@chromium.org>
Date: Mon Apr 16 02:56:55 2018

tast: Move a few flags out of runCmd.

Move the -checkdeps (renamed to -checkbuilddeps) and
-sysinfo flags from runCmd to the cmd/tast/run package
(that's shared with listCmd). -sysinfo is irrelevant to (and
hidden for) listCmd, but -checkbuilddeps applies to both
the run and list commands. Also stop exporting some
build-related fields from run.Config.

-buildtype still lives in runCmd (with listCmd having its
own -type flag), but this functionality will likely also be
consolidated into the run package in a later change.

BUG= chromium:831849 
TEST=unit tests pass; also manually verified that run and
     list commands still work with -build and -build=false
     for local and remote tests

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

[modify] https://crrev.com/eb4d352248edb84c6aed3bf2ca133b0c7e4c51c5/src/chromiumos/cmd/tast/list_cmd.go
[modify] https://crrev.com/eb4d352248edb84c6aed3bf2ca133b0c7e4c51c5/src/chromiumos/cmd/tast/run/config.go
[modify] https://crrev.com/eb4d352248edb84c6aed3bf2ca133b0c7e4c51c5/src/chromiumos/cmd/tast/run_cmd.go
[modify] https://crrev.com/eb4d352248edb84c6aed3bf2ca133b0c7e4c51c5/src/chromiumos/cmd/tast/run/local.go
[modify] https://crrev.com/eb4d352248edb84c6aed3bf2ca133b0c7e4c51c5/src/chromiumos/cmd/tast/run/remote.go

Project Member

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

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

commit 3b445c480ad65e4b872ee8945a783d2081cef930
Author: Daniel Erat <derat@chromium.org>
Date: Tue Apr 17 01:02:27 2018

tast: Move -buildtype to cmd/tast/run and add run.Run.

Make the cmd/tast/run package responsible for determining
which type(s) of tests to build and run/list. This involves
moving runCmd's -buildtype flag to the run package, dropping
listCmd's -type flag, and hiding run.Local and run.Remote
behind a single run.Run function.

With this change, the run command's behavior is unchanged,
but the list command now lists both local and remote tests
for -build=false.

BUG= chromium:831849 
TEST=unit tests pass; also manually verified that run works
     as before and "list -build=false" lists all tests

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

[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run_cmd_test.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run/remote.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run/results_test.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run_wrapper_test.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run_wrapper.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run_cmd.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run/local.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run/local_test.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/list_cmd.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run/sys_info.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run/sys_info_test.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run/remote_test.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run/config.go
[modify] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/list_cmd_test.go
[add] https://crrev.com/3b445c480ad65e4b872ee8945a783d2081cef930/src/chromiumos/cmd/tast/run/run.go

Project Member

Comment 8 by bugdroid1@chromium.org, Apr 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/9ac588d1c12071d70f1a496b59443ea86c4940ac

commit 9ac588d1c12071d70f1a496b59443ea86c4940ac
Author: Daniel Erat <derat@chromium.org>
Date: Thu Apr 19 20:14:10 2018

autotest: Make tast report missing tests.

Update the tast server test to use tast's "list" command
before running tests so it can report any tests that failed
to run in the status.log file later.

BUG= chromium:831849 
TEST=hacked up tast.py to add an additional test to the
     expected-to-run last and verified that it is reported
     in the status.log file at the end of the run

Change-Id: Iec5afdf7b4230d0096308a4f0c95d5c3aa02b30c
Reviewed-on: https://chromium-review.googlesource.com/1018588
Reviewed-by: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/9ac588d1c12071d70f1a496b59443ea86c4940ac/client/common_lib/utils.py
[modify] https://crrev.com/9ac588d1c12071d70f1a496b59443ea86c4940ac/server/site_tests/tast/tast.py

Project Member

Comment 9 by bugdroid1@chromium.org, May 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ece1bad856e9ef34b2e0d59f4dd2099fbb3d0090

commit ece1bad856e9ef34b2e0d59f4dd2099fbb3d0090
Author: Daniel Erat <derat@chromium.org>
Date: Tue May 15 18:28:03 2018

autotest: Improve handling of Tast execution failures.

Improve the "tast" server test's handling of failed
executions of the tast command:

Register the post-run hook earlier to ensure that not-run
tests will be reported in status.log even if tast fails
while running tests. (Note that the initial "tast list"
command still needs to succeed in order for us to get the
list of expected tests in the first place.)

Also include the final line of the command's stdout in the
first line of the TestFail exception make it easier to see
what went wrong.

BUG= chromium:770439 , chromium:831849 , chromium:842453 
TEST=modified the tast executable to fail before writing
     results.json and verified that status.log contains
     "Test was not run" FAIL results for all Tast tests and
     that the main "tast" result starts with "Failed to run
     tast (Failed to write results: intentional failure):
     Command ..."

Change-Id: I81bfa3eca80188fbe5c2e31316938fa2623a3156
Reviewed-on: https://chromium-review.googlesource.com/1056049
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/ece1bad856e9ef34b2e0d59f4dd2099fbb3d0090/server/site_tests/tast/tast.py

Comment 10 by derat@chromium.org, May 25 2018

Status: Fixed (was: Assigned)

Sign in to add a comment