New issue
Advanced search Search tips

Issue 882729 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Feature



Sign in to add a comment

Reconsider faillog API

Project Member Reported by nya@chromium.org, Sep 11

Issue description

I propose to enforce faillog to be used by *all* local tests.


Background:

We introduced faillog package to save some logs on test failures in  issue 856540 .

My initial intention was that faillog is used in all local tests. When diagnosing failures, it's often unclear which logs are useful for debugging --- for example, some failures are often very obvious by a single screenshot because error messages are shown on display.

Currently faillog saves a screenshot and process list, but I expect more logs to be added later.

The problem is that current faillog API is opt-in and does not enforce to be used by all local tests. People may forget to enable it, or choose not to use it because it sounds not very useful for their particular tests --- but no one knows which logs are useful for debugging before failures happen, actually.


Challenges:

I want faillog to be invoked even if each test function does not mention it at all. However this needs changes to Tast's test runner, so we need design discussion.

 
Cc: hidehiko@chromium.org
I think I'm supportive of running faillog unconditionally. We should probably make the screenshot code less spammy, though (at least as long as it's known to fail on some boards). :-)

Do you think we should give tests (or other packages) the ability to register additional data or collection functions with faillog? For example, should testexec automatically save uncaptured command output in the event of a test failure? We might need to figure out how to make it not be too noisy in case someone e.g. runs a command via testing.Poll.
Owner: nya@chromium.org
Status: Started (was: Untriaged)
Thanks! I'll try making a change.

Maybe some tests want to explicitly configure faillog as you described. Let's start from simple one and add configuration once we find some tests want to do so.

FYI, my current rough idea:
- Introduce hook registry in tast/testing.
- Register faillog as a hook in cros bundle main.

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 21

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

commit 042eecda77ad7b131aa7d78735bfe58935554e30
Author: Shuhei Takahashi <nya@chromium.org>
Date: Fri Sep 21 10:38:12 2018

tast-tests: Remove faillog.

Faillog is now automatically gathered by CL:1229543.

CQ-DEPEND=CL:1229543
BUG= chromium:882729 
TEST=tast run DUT example.Fail # faillog is still saved

Change-Id: I3c073f4c54c0f70db6bbfdf4d20463c2f5a45f2d
Reviewed-on: https://chromium-review.googlesource.com/1229817
Commit-Ready: Shuhei Takahashi <nya@chromium.org>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/ui/virtual_keyboard_omnibox.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/graphics/screenshot.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/example/pass.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/audio/device_play.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/video/webrtc/webrtc.go
[delete] https://crrev.com/288161fe3a73250d9e6ef8696b99df6fc620e5a3/src/chromiumos/tast/local/faillog/faillog.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/arc/boot_forever.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/arc/intent_forward.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/security/log_perms.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/example/dbus.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/video/play/play.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/ui/chrome_login.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/vm/crostini_start_everything.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/ui/mash_login.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/arc/boot.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/example/fail.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/arc/downloads.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/arcapp/sample.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/example/perf.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/platform/tpm_responsive.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/platform/check_processes.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/vm/start_crosvm.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/power/check_status.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/audio/device_record.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/security/sandboxed.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/ui/chrome_crash_logged_in.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/ui/chrome_crash_not_logged_in.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/ui/single_process_mash_login.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/example/data_files.go
[modify] https://crrev.com/042eecda77ad7b131aa7d78735bfe58935554e30/src/chromiumos/tast/local/bundles/cros/security/gpu_sandboxed.go

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 21

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

commit c5c3eeb06094befd8b0094bd60ba2d04e6a24ae1
Author: Shuhei Takahashi <nya@chromium.org>
Date: Fri Sep 21 10:38:11 2018

tast: Run faillog automatically for local tests.

testCleanupFunc is added to runConfig to gather faillog
automatically at the end of tests.

Note that testing.State passed to testCleanupFunc has limited
capability (e.g. no logging).

BUG= chromium:882729 
TEST=fast_build.sh -T

Change-Id: I326daf68306bc48faef0c4570fef52dc780a8055
Reviewed-on: https://chromium-review.googlesource.com/1229543
Commit-Ready: Shuhei Takahashi <nya@chromium.org>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/c5c3eeb06094befd8b0094bd60ba2d04e6a24ae1/src/chromiumos/tast/bundle/local_test.go
[modify] https://crrev.com/c5c3eeb06094befd8b0094bd60ba2d04e6a24ae1/src/chromiumos/tast/bundle/bundle_test.go
[add] https://crrev.com/c5c3eeb06094befd8b0094bd60ba2d04e6a24ae1/src/chromiumos/tast/faillog/faillog.go
[modify] https://crrev.com/c5c3eeb06094befd8b0094bd60ba2d04e6a24ae1/src/chromiumos/tast/bundle/bundle.go
[modify] https://crrev.com/c5c3eeb06094befd8b0094bd60ba2d04e6a24ae1/src/chromiumos/tast/bundle/local.go

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 26

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

commit 5092233dad5b9200856e4dfbd6b1cb1478021c5a
Author: Shuhei Takahashi <nya@chromium.org>
Date: Wed Sep 26 17:32:57 2018

tast: Run test setup/cleanup funcs inside Run.

Now setup/cleanup funcs are run in the exactly same testing.State
as test main functions, allowing to call Log for example. Also,
now cleanup function can detect an error if a test main function
was panicked.

CQ-DEPEND=CL:1229543
BUG= chromium:882729 
TEST=fast_build.sh -T

Change-Id: I2a60a8e78bdd98fc4ca721a5595ca5d6c338ec76
Reviewed-on: https://chromium-review.googlesource.com/1233139
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/5092233dad5b9200856e4dfbd6b1cb1478021c5a/src/chromiumos/tast/bundle/bundle.go
[modify] https://crrev.com/5092233dad5b9200856e4dfbd6b1cb1478021c5a/src/chromiumos/tast/bundle/local.go
[modify] https://crrev.com/5092233dad5b9200856e4dfbd6b1cb1478021c5a/src/chromiumos/tast/bundle/remote.go
[modify] https://crrev.com/5092233dad5b9200856e4dfbd6b1cb1478021c5a/src/chromiumos/tast/testing/test_test.go
[modify] https://crrev.com/5092233dad5b9200856e4dfbd6b1cb1478021c5a/src/chromiumos/tast/testing/state.go
[modify] https://crrev.com/5092233dad5b9200856e4dfbd6b1cb1478021c5a/src/chromiumos/tast/testing/test.go
[modify] https://crrev.com/5092233dad5b9200856e4dfbd6b1cb1478021c5a/src/chromiumos/tast/testing/state_test.go
[modify] https://crrev.com/5092233dad5b9200856e4dfbd6b1cb1478021c5a/src/chromiumos/tast/bundle/bundle_test.go

Sign in to add a comment