New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 791878 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Task

Blocking:
issue 784944



Sign in to add a comment

tast: Reorganize code

Project Member Reported by derat@chromium.org, Dec 5 2017

Issue description

As discussed a while back on https://crrev.com/c/692715: the organization of the various code used by tast is currently pretty cumbersome. For example, the code for the "tast" command lives in a directory named "cmd", which runs contrary to the usual practice in Go.

Here are some suggestions I made on that change and Rahul's reply:

----

> in src/platform/tast repository's src/ directory:
> chromiumos/tast/cmd/tast (currently just chromiumos/tast/cmd)
> chromiumos/tast/cmd/tast/build (support package for tast command)
> chromiumos/tast/common/... (shared support packages)

How about in src/platform/tast repository's src/ directory:
chromiumos/cmd/tast (currently just chromiumos/tast/cmd)
chromiumos/tast/build (support package for tast command)
chromiumos/tast/common/... (shared support packages)

i.e. the commands can all be in "chromiumos/cmd/" namespace, and all tast related importable packages under "chromiumos/tast/..."

> in src/platform/tast-tests repository's src/ directory:
> chromiumos/tast/cmd/local_tests (currently just chromiumos/tast/local)
> chromiumos/tast/cmd/remote_tests (currently just chromiumos/tast/remote)
> chromiumos/tast/local/chrome (support package for local_tests)
> chromiumos/tast/local/tests/ui (local test package)
> chromiumos/tast/remote/tests/power (remote test package)

Same as above, how about in src/platform/tast-tests repository's src/ directory:
chromiumos/cmd/local_tests (currently just chromiumos/tast/local)
chromiumos/cmd/remote_tests (currently just chromiumos/tast/remote)
chromiumos/tast/local/chrome (support package for local_tests)
chromiumos/tast/local/tests/ui (local test package)
chromiumos/tast/remote/tests/power (remote test package)

----

I'm thinking about instead doing the following:

chromiumos/cmd/tast          tast main pkg
chromiumos/cmd/tast/...      packages used by tast command
chromiumos/cmd/local_tests   local_tests main pkg
chromiumos/cmd/remote_tests  remote_tests main pkg
chromiumos/tast/local/...    local tests and related packages
chromiumos/tast/remote/...   remote tests and related packages
chromiumos/tast/...          all other packages shared between executables

(Note that test-related code will probably move around more for  issue 784944 , but I'd like to sort this out first.)
 

Comment 1 by derat@chromium.org, Dec 5 2017

Rahul, when testing local changes to move code around as described here, I noticed what seemed like some problems with the way we unpack source for building Go-based packages. Specifically, when I emerged the tast-common package, the older copy of the code appeared to still be sitting around in /build/<board>/usr/lib/gopath/src/chromiumos/tast/common (after I'd moved the files to .../src/chromiumos/tast in the repository), and the old files got built as well and caused trouble when trying to install the package:

...
ok      chromiumos/tast/control 0.029s
ok      chromiumos/tast/host    0.419s
?       chromiumos/tast/host/test       [no test files]
ok      chromiumos/tast/runner  3.084s
ok      chromiumos/tast/testing 0.058s
ok      chromiumos/tast/testing/attr    0.029s
?       chromiumos/tast/testutil        [no test files]
ok      chromiumos/tast/common/control  0.017s
ok      chromiumos/tast/common/host     0.335s
?       chromiumos/tast/common/host/test        [no test files]
ok      chromiumos/tast/common/runner   0.019s
ok      chromiumos/tast/common/testing  0.058s
ok      chromiumos/tast/common/testing/attr     0.016s
?       chromiumos/tast/common/testutil [no test files]
 * Running stacked hooks for post_src_test
 *    asan_check ...                                                                                   [ ok ]

>>> Install tast-common-9999 into /build/lumpy/tmp/portage/chromeos-base/tast-common-9999/image/ category chromeos-base
 * Installing "chromiumos/tast/control"
 * Installing "chromiumos/tast/host"
 * Installing "chromiumos/tast/host/test"
 * Installing "chromiumos/tast/runner"
 * Installing "chromiumos/tast/testing"
 * Installing "chromiumos/tast/testing/attr"
 * Installing "chromiumos/tast/testutil"
 * Installing "chromiumos/tast/common/control"
find: `/build/lumpy/tmp/portage/chromeos-base/tast-common-9999/work/tast-common-9999/src/chromiumos/tast/common/control': No such file or directory
 * Installing "chromiumos/tast/common/host"
find: `/build/lumpy/tmp/portage/chromeos-base/tast-common-9999/work/tast-common-9999/src/chromiumos/tast/common/host': No such file or directory
...

----

How's this supposed to work? I'm doing this in tast-common, so I'm not surprised it's building both the old and new files:

 CROS_GO_PACKAGES=(
-       "chromiumos/tast/common/..."
+       "chromiumos/tast/..."
 )

It seems like we might need to clear out the old files before building tests and installing, though. (The build worked after I manually deleted the old files from .../usr/lib/gopath/src/chromiumos.)

Comment 2 by derat@chromium.org, Dec 5 2017

To make that question less abstract, here are the changes in question (might need a bit more work; I didn't spend much time on them or double-check anything beyond that the code builds and tests pass):

https://crrev.com/c/807842
https://crrev.com/c/807843
https://crrev.com/c/807753
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/633b1cb2b134320039dd01e5c38c31be34bd61df

commit 633b1cb2b134320039dd01e5c38c31be34bd61df
Author: Rahul Chaudhry <rahulchaudhry@chromium.org>
Date: Wed Dec 06 02:00:38 2017

cros-go.eclass: ignore GOPATH when expanding wildcard in CROS_GO_PACKAGES

When expanding "..." wildcard in CROS_GO_PACKAGES, only the
local workspace should be considered. Packages from system-
wide GOPATH should not be included in that list.

BUG= chromium:791878 
TEST='emerge chromeos-base/tast-common' works with changes from
     https://chromium-review.googlesource.com/807842 and
     https://chromium-review.googlesource.com/807753.

Change-Id: Ie4cbbcbf8a6e166f79325dc26ad227fdfbdc5b67
Reviewed-on: https://chromium-review.googlesource.com/808752
Commit-Ready: Rahul Chaudhry <rahulchaudhry@chromium.org>
Tested-by: Rahul Chaudhry <rahulchaudhry@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/633b1cb2b134320039dd01e5c38c31be34bd61df/eclass/cros-go.eclass

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/fcff2cbba507c347d20da3454cd629415dc9ea7a

commit fcff2cbba507c347d20da3454cd629415dc9ea7a
Author: Rahul Chaudhry <rahulchaudhry@chromium.org>
Date: Wed Dec 06 11:45:54 2017

cros-go.eclass: ignore GOPATH when expanding wildcard in CROS_GO_TEST

Similar to CROS_GO_PACKAGES, when expanding "..." wildcard in
CROS_GO_TEST, only the local workspace should be considered.
Packages from the system-wide GOPATH should not be picked up
in the list of packages to test.

BUG= chromium:791878 
TEST='emerge FEATURES=test chromeos-base/tast-common' works with changes
     from https://chromium-review.googlesource.com/807842
     and https://chromium-review.googlesource.com/807753
     and does not pick up older "chromiumos/tast/common/..."
     packages from system-wide GOPATH for testing.

Change-Id: I4f923dda09486bac8b31fb419bd093c8837e0f88
Reviewed-on: https://chromium-review.googlesource.com/810019
Commit-Ready: Rahul Chaudhry <rahulchaudhry@chromium.org>
Tested-by: Rahul Chaudhry <rahulchaudhry@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/fcff2cbba507c347d20da3454cd629415dc9ea7a/eclass/cros-go.eclass

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 7 2017

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

commit e1499fd449f5a80d0ff95b0cb1ef8d95f4976751
Author: Daniel Erat <derat@chromium.org>
Date: Thu Dec 07 23:22:45 2017

tast-tests: Move files around.

Move executables to chromiumos/cmd/local_tests and
chromiumos/cmd/remote_tests, and move other packages under
chromiumos/tast/local/ and chormiumos/tast/remote.

BUG= chromium:791878 
TEST=still builds
CQ-DEPEND=I702870e6efe13dd1365eccd38deac45082fbe2eb
CQ-DEPEND=I6c065c7a2a422b8a4ee3b5dd467928a5c01f533f

Change-Id: I58fa9ef006f9a0976c370252ce29f1c23a8db724
Reviewed-on: https://chromium-review.googlesource.com/807843
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Rahul Chaudhry <rahulchaudhry@chromium.org>

[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/logs/logs_test.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/tests/power/check_status.go
[rename] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/cmd/local_tests/data_files_test.go
[rename] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/cmd/local_tests/data_files.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/remote/tests/power/reboot.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/tests/ui/chrome_sanity.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/remote/dut/dut.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/tests/example/dbus.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/tests/ui/mash_login.go
[rename] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/cmd/local_tests/main.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/tests/security/log_perms.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/chrome/arc.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/tests/example/data_files.go
[rename] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/cmd/remote_tests/main.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/chrome/extensions_test.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/chrome/cryptohome.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/README.md
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/chrome/conn.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/chrome/chrome_test.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/tests/ui/arc_sanity.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/chrome/chrome.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/tests/example/pass.go
[modify] https://crrev.com/e1499fd449f5a80d0ff95b0cb1ef8d95f4976751/src/chromiumos/tast/local/tests/example/fail.go

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/8d5755f6da3e8f4961d6f750e1d519baa5ba0421

commit 8d5755f6da3e8f4961d6f750e1d519baa5ba0421
Author: Daniel Erat <derat@chromium.org>
Date: Thu Dec 07 23:22:45 2017

tast-*: Move files around.

Update the location of source code used by the tast-cmd,
tast-common, tast-local-tests, and tast-remote-tests
packages.

BUG= chromium:791878 
TEST=packages build with FEATURES=test
CQ-DEPEND=I702870e6efe13dd1365eccd38deac45082fbe2eb
CQ-DEPEND=I58fa9ef006f9a0976c370252ce29f1c23a8db724
CQ-DEPEND=Ie4cbbcbf8a6e166f79325dc26ad227fdfbdc5b67

Change-Id: I6c065c7a2a422b8a4ee3b5dd467928a5c01f533f
Reviewed-on: https://chromium-review.googlesource.com/807753
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Rahul Chaudhry <rahulchaudhry@chromium.org>

[modify] https://crrev.com/8d5755f6da3e8f4961d6f750e1d519baa5ba0421/chromeos-base/tast-common/tast-common-9999.ebuild
[modify] https://crrev.com/8d5755f6da3e8f4961d6f750e1d519baa5ba0421/chromeos-base/tast-remote-tests/tast-remote-tests-9999.ebuild
[modify] https://crrev.com/8d5755f6da3e8f4961d6f750e1d519baa5ba0421/chromeos-base/tast-local-tests/tast-local-tests-9999.ebuild
[modify] https://crrev.com/8d5755f6da3e8f4961d6f750e1d519baa5ba0421/chromeos-base/tast-cmd/tast-cmd-9999.ebuild

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 7 2017

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

commit 71c60640e8df3d75d57a4fa1c553670ed0f7a219
Author: Daniel Erat <derat@chromium.org>
Date: Thu Dec 07 23:22:45 2017

tast: Move files around.

Move the tast command from chromiumos/tast/cmd to
chromiumos/cmd/tast, and move all other packages from
chromiumos/tast/common/ to chromiumos/tast/.

BUG= chromium:791878 
TEST=still builds
CQ-DEPEND=I58fa9ef006f9a0976c370252ce29f1c23a8db724
CQ-DEPEND=I6c065c7a2a422b8a4ee3b5dd467928a5c01f533f

Change-Id: I702870e6efe13dd1365eccd38deac45082fbe2eb
Reviewed-on: https://chromium-review.googlesource.com/807842
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Rahul Chaudhry <rahulchaudhry@chromium.org>

[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/logging/simple.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/main.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/run/print.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/timing/timing_test.go
[modify] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/docs/writing_tests.md
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/testing/registry_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/build/build_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/run/local_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/timing/timing.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/build/config.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/display/term.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/build/portage.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/control/control_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/run/line.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/run_cmd.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/run/line_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/build/portage_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/testing/test_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/testing/global.go
[modify] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/docs/test_attributes.md
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/logging/logger.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/run/config.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/runner/runner.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/host/ssh.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/run/results_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/runner/runner_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/run/results.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/testing/attr/attr_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/display/display.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/display/display_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/host/test/ssh_server.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/run/local.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/host/test/ssh_keys.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/testing/attr/attr.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/testutil/files.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/host/ssh_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/build/build.go
[modify] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/README.md
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/logging/simple_test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/run/remote.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/testing/registry.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/logging/fancy.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/chroot.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/build_cmd.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/cmd/tast/list_cmd.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/testing/test.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/testing/state.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/control/control.go
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/fast_build.sh
[rename] https://crrev.com/71c60640e8df3d75d57a4fa1c553670ed0f7a219/src/chromiumos/tast/testing/state_test.go

Comment 8 by derat@chromium.org, Dec 7 2017

Status: Verified (was: Started)

Comment 9 by derat@chromium.org, Jan 23 2018

Components: Tests>Tast

Sign in to add a comment