New issue
Advanced search Search tips

Issue 890546 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 1
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Tast test functions should take explicit context.Context parameters

Project Member Reported by derat@chromium.org, Sep 29

Issue description

https://golang.org/pkg/context/ is clear on the topic of storing a context.Context in a struct: "Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx:"

When I added the testing.State struct, I thought that it was a special case since it's passed into test functions and doesn't outlive any individual test, so I went ahead and made it store a context (two contexts, actually) and expose a Context() function that tests can call.

I'm starting to think that that was a mistake, though. We now pass the same testing.State to several different functions (test functions, setup functions, cleanup functions), and as a result, they all share the same context and the same deadline. It's likely that we'll want to pass it to even more places in the future (e.g. preconditions). I think we need the ability to pass different contexts in different places.

I've also noticed that many (most?) tests contain a statement like:

  ctx := s.Context()

This suggests to me that the context is used frequently enough that it should be a test function argument, e.g.

  func MyTest(ctx context.Context, s *testing.State) {

That saves a line of boilerplate in each test (at the cost of a longer function signature).

If we want to change this, it'll be somewhat painful, but I think it can be done relatively safely with the following steps:

a) Add a new field to testing.Test:
   FuncCtx func(context.Context, *testing.State)
b) Update Test.Run to invoke FuncCtx instead of Func if the
   former is non-nil, and to pass s.tctx to it.
c) Update all tests to set FuncCtx instead of Func.
d) Update Test.Func to also take a context.
e) Update tests to set Func instead of FuncCtx.
f) Delete FuncCtx and State.Context.

It would also be possible to just do it in two simultaneous changes to tast and tast-tests, but that would be dependent on nobody trying to add new tests and may be hard to land.

Does this sound reasonable?
 
Status: Started (was: Assigned)
After trying this out (in https://crrev.com/c/1253190 and https://crrev.com/c/1252568), I think it's probably worth the temporary pain.
I totally agree with rationals. Let's do this early.

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 1

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

commit 80fbfd11270e08b1ba89f7e6a86e8e9f88a211d2
Author: Daniel Erat <derat@chromium.org>
Date: Mon Oct 01 16:50:31 2018

tast: Make Test.Run construct its own State.

Introduce a new TestConfig struct in the testing package and
pass it to Test.Run instead of passing a State struct.

This improves encapsulation (there was no reason that the
bundle package needed to know about testing.State) and
readability (the NewState function had too many arguments).

It also makes it possible for future changes to refactor
Test.Run to e.g. perform additional preliminary work without
it counting against the test's timeout.

Also make a few related cleanups: perform checks in
State.Meta, and create test output dirs in Test.Run instead
of in the bundle package.

BUG= chromium:890546 
TEST=updated unit tests; also manually verified that local
     and remote tests still work, and that power.CheckStatus
     returns its output file and that meta.* still pass

Change-Id: I94c8a528040d3b71dfa04aa3f8983c17199496c1
Reviewed-on: https://chromium-review.googlesource.com/1252933
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Trybot-Ready: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/80fbfd11270e08b1ba89f7e6a86e8e9f88a211d2/src/chromiumos/tast/testing/test_test.go
[modify] https://crrev.com/80fbfd11270e08b1ba89f7e6a86e8e9f88a211d2/src/chromiumos/tast/testing/state_test.go
[modify] https://crrev.com/80fbfd11270e08b1ba89f7e6a86e8e9f88a211d2/src/chromiumos/tast/testing/state.go
[modify] https://crrev.com/80fbfd11270e08b1ba89f7e6a86e8e9f88a211d2/src/chromiumos/tast/bundle/bundle.go
[modify] https://crrev.com/80fbfd11270e08b1ba89f7e6a86e8e9f88a211d2/src/chromiumos/tast/testing/test.go

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 1

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

commit f5b02584aab481821a3ee30e57438bc97eb1def5
Author: Daniel Erat <derat@chromium.org>
Date: Mon Oct 01 16:50:54 2018

tast: Make test functions take explicit context args.

Update testing.Test's Func field to take a
func(context.Context, *State) rather than a func(*State),
and remove the Context method from State.

Passing explicit contexts reduces boilerplate "ctx :=
s.Context()" statements in tests, enables future framework
changes to pass different contexts to test functions vs.
test setup and cleanup functions, and aligns with Go's
guidance to not store contexts in structs (which I should
have listened to in the first place :-/).

BUG= chromium:890546 
TEST=unit tests pass; also manually verified that local and
     remote tests still build and run
CQ-DEPEND=I98971d27988f0b163da5c8a1072024d8736b8816

Change-Id: I096dc823d4d03ea9ff6ca31a6580d5638cdced66
Reviewed-on: https://chromium-review.googlesource.com/1253190
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Trybot-Ready: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/bundle/local_test.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/faillog/faillog.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/runner/runner_test.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/bundle/remote_test.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/bundle/bundle.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/docs/writing_tests.md
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/bundle/remote.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/testing/test_test.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/docs/quickstart.md
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/testing/state.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/bundle/args_test.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/testing/test.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/testing/state_test.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/bundle/bundle_test.go
[modify] https://crrev.com/f5b02584aab481821a3ee30e57438bc97eb1def5/src/chromiumos/tast/testing/registry_test.go

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 1

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

commit e04340a958c558bbd71e13e8e607d2a47d18fa81
Author: Daniel Erat <derat@chromium.org>
Date: Mon Oct 01 16:50:56 2018

tast-tests: Update all test funcs to take context args.

Update all test functions to take explicit context.Context
args instead of accessing contexts via a Context method on
testing.State. This needs to land in conjunction with a
corresponding framework change.

BUG= chromium:890546 
TEST=all tests build successfully
CQ-DEPEND=I096dc823d4d03ea9ff6ca31a6580d5638cdced66

Change-Id: I98971d27988f0b163da5c8a1072024d8736b8816
Reviewed-on: https://chromium-review.googlesource.com/1252568
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Trybot-Ready: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/meta/local_files.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/virtual_keyboard_omnibox.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/remote/bundles/cros/meta/tastrun/run.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/printer/cupsd.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/play_h264.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/webrtc_peer_connection_with_camera_h264.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/graphics/screenshot.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/platform/histograms.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/play_vp8.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/audio/device_play.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/webrtc/webrtc.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/remote/bundles/cros/power/reboot.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/arc/boot_forever.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/security/mount_symlink.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/chrome_respawn.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/arc/intent_forward.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/audio/device_record.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/security/log_perms.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/remote/bundles/cros/example/reconnect_to_dut.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/network/default_profile.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/vm/subtest/linux_package_info.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/example/dbus.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/play/play.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/chrome_login.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/vm/crostini_start_everything.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/mash_login.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/arc/boot.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/example/fail.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/chrome_login_forever.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/remote/bundles/cros/meta/remote_files.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/vm/subtest/webserver.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/arc/downloads.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/example/data_files.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/arcapp/sample.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/platform/ml_service_bootstrap.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/security/gpu_sandboxed.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/vm/subtest/launch_terminal.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/webrtc_camera_perf.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/session_manager_respawn.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/example/perf.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/platform/tpm_responsive.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/platform/check_processes.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/security/open_fds.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/arcapp/apptest/apptest.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/vm/start_crosvm.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/power/check_status.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/exceptions.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/respawn/respawn.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/webrtc_peer_connection_with_camera_vp8.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/security/selinux_file_label_with_chrome.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/meta/local_panic.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/example/pass.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/security/selinux_file_label.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/security/sandboxed.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/vm/container.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/remote/bundles/cros/meta/list_tests.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/chrome_crash_logged_in.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/webrtc_camera.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_app_from_terminal.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/chrome_crash_not_logged_in.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/virtual_keyboard_typing.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/remote/bundles/cros/meta/run_tests.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/ui/single_process_mash_login.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/audio/device/device.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/audio/microphone.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/webrtc_peer_connection_with_camera_vp8_perf.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/vm/crostini_files.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/play_vp9.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/vm/subtest/launch_browser.go
[modify] https://crrev.com/e04340a958c558bbd71e13e8e607d2a47d18fa81/src/chromiumos/tast/local/bundles/cros/video/webrtc_peer_connection_with_camera_h264_perf.go

Status: Verified (was: Started)

Sign in to add a comment