Makes sense. I thought about this a bit when looking at some video tests but didn't write any code.

There's nothing Autotest-specific about the YAML files, right? They look like they're just installed by chromeos-base/autotest-capability packages in various board overlays.

Yes. These YAML files just provides information on which devices (e.g. camera, hw encoder/decoder) can be used on each board, and aren't specific to autotest. 
Since chromeos-base/autotest-capability is enabled on each board by default, we should just reuse the YAMLs for Tast.

derat@, how do you think we should use a program for device capability?

I think we have 2 options:
(1) Each testcase uses the function inside it and always returns "success" if the device doesn't have required hardware.
(2) Test manager uses the function and each test won't run on a device that doesn't have required hardware. 
(I'm not sure if such a test manager exists though)

Please refer "BackGround" in the following document.
I think there is no way except autotest-capability in the current CrOS codebase to avoid these issues.
https://docs.google.com/document/d/1ILLTN9MBG9-6WgzuVmm0FH8US188tjQ52_WACAR-2WU/edit#heading=h.csxfuo33h3s7

After re-thinking about #7, I think that we should choose the option (1) now. 
This is because we need to send a test binary to DUT to read YAMLs files anyway. If we choose (2), we need two binaries, one for reading YAMLs and the other for test, which are not smart.
Moreover, we can also check whether YAMLs are properly placed by the option (1).

Just to make sure I understand correctly, these capabilities are tied to the system image that is being used, right? I'm assuming that that's currently the case since they're communicated via a Portage package that's presumably baked into the system image.

Will you ever need to use the same system image on multiple DUTs that have different capabilities (e.g. using unibuild)?

This feels like it may be conceptually similar to Tast's existing "software dependencies", which are used to skip tests when a system image that is incapable of running them is being used (e.g. Android tests on a system image that doesn't support Android). Please see http://doc/133fqdEFbUZ6g0IhHFtPQOvl2YHFJg1_QBne8p3rTgz8 for some background, and also the public documentation at https://chromium.googlesource.com/chromiumos/platform/tast/+/HEAD/docs/test_dependencies.md.

The existing support in Tast is based off of a mapping from feature names boolean expressions that reference USE flags. It would be possible to add support for specifying additional features based on the content of these YAML files.

I'll think about this a bit more.

Hiro and I just chatted about this over VC. The documentation at https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/chromeos-base/autotest-capability-default/ is very helpful for understanding how autotest-capability works, but in short:

- YAML files are installed to /usr/local/etc/autotest-capability.
- managed-capabilities.yaml lists all capabilities that can be set.
- The remaining files are read in-order, with their listed capabilities overlaid.
- Previously-set capabilities can be unset using a "no " or "disabled " prefix.

Unfortunately, capabilities aren't tied entirely to software. They can also be dependent on "detectors", which are Python code that is executed to determine capabilities at runtime. I'm not sure if http://cs/chromeos_public/src/third_party/autotest/files/client/cros/video/detectors/ is the full set of detectors or if there are more elsewhere.

I initially thought that this should be implemented using Tast software features/dependencies as suggested above, but after learning more about it, I'm less sure. These capabilities are actually hardware-dependent in some cases (i.e. not strictly tied to the system image / board), and the current implementation is only used by video tests, I think.

It also sounds like the current implementation is non-ideal in some ways, as tests can sometimes get scheduled on DUTs that are incapable of running them (because Autotest doesn't know about detector-specified capabilities -- is that correct?).

As such, I'm now thinking that it might be better to just implement this entirely within the video tests at runtime instead of building it into the framework. This would probably require adding a Skip() or similar method to testing.State that tests can call at runtime. I'd wanted to avoid this (since runtime checks are slower than just not running the test at all), but I don't know if I have a better solution. nya@, I'm curious about your opinion here.

It would also be good to try to choose an approach that's compatible with infra's future plans around test scheduling.

In any case, I think that Hiro is going to write a brief doc with a proposed implementation. We can discuss this further there.

I misunderstood the timing of deciding a dependencies list in tast_use_flags.txt.
Currently, tast_use_flags.txt is deterministic in building image.
Is it possible to overwrite it after providing an image, by running a code like DeviceCapability?
https://chromium.googlesource.com/chromiumos/third_party/autotest/+/master/client/cros/video/device_capability.py

> I'm not sure if http://cs/chromeos_public/src/third_party/autotest/files/client/cros/video/detectors/ is the full set of detectors or if there are more elsewhere.

That's all detectors.

My original suggestion wasn't to use tast_use_flags.txt for this (since there aren't any USE flags describing these capabilities, IIUC). Rather, I was saying that we could introduce similar code that reads autotest-capability's YAML files and registers additional software features that tests can list as dependencies.

After thinking about it more (as described in #11), I thought it would probably be better to keep this code within the video tests that use it instead of adding it to the core Tast code, as it's currently only used by video tests (I think) and has issues around test scheduling that may prevent us from wanting to use it more widely in the future.

But I still have some reluctance around adding a way for tests to be skipped at runtime. I'm writing some code now to see what adding autotest-capability support to Tast would look like. I hope you don't mind if I steal this bug.

This seems pretty straightforward to do. I've uploaded a work-in-progress change at https://crrev.com/c/1234592. It implements (I think) pretty much all of the capability-reading/detecting code that would be needed. I didn't write the trivial bits to run lspci and lscpu or connect it to local_test_runner yet, but that wouldn't be much more code.

If we go this route, I'd probably have this code synthesize new dependencies with names like "autotest:hw_dec_vp8_1080_30".

It still feels a bit strange to have this be part of Tast instead of making it internal to video tests, but at the same time, it's arguably cleaner to make use of the existing dependencies system instead of adding a new mechanism to let tests decide to be skipped at runtime. I'm also hopeful that the implementation is separate enough from the rest of the code that it would be easy to remove it if/when we have something else.

I think it's still important to have a discussion with infra to understand their plans for test scheduling, as this approach will still have your existing problems around missing coverage when a test is scheduled to run on a DUT that doesn't support it.

Wow, thanks Daniel for working and scrutinizing a lot for this.

> I'm writing some code now to see what adding autotest-capability support to Tast would look like. I hope you don't mind if I steal this bug.

Of course not. I really appreciate you are working for this.

> It still feels a bit strange to have this be part of Tast instead of making it internal to video tests, but at the same time, it's arguably cleaner to make use of the existing dependencies system instead of adding a new mechanism to let tests decide to be skipped at runtime. I'm also hopeful that the implementation is separate enough from the rest of the code that it would be easy to remove it if/when we have something else.

I think video dependency is a rare case to have to create dependencies with software dependencies and hardware dependencies (device-skew). I don't know there is other use cases than video tests (I will ask camera team if camera capabilities can be resolved by USE flags).
Thus, I think it is good to put the code for autotest-capability in local_tests/video or somewhere independent on tast main system, in order to avoid making tast system complicated.

> I think it's still important to have a discussion with infra to understand their plans for test scheduling, as this approach will still have your existing problems around missing coverage when a test is scheduled to run on a DUT that doesn't support it.

Definitely. I also don't like to run a test which will be resulted in SKIP. Let's find the best way discussing infra developers.

Thanks,
-Hiro

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

commit cf4ec05b94d19bd94bba17c6bedf20a80cfc6c68
Author: Daniel Erat <derat@chromium.org>
Date: Fri Sep 21 07:51:23 2018

Add dev-go/yaml and dev-go/check packages.

Add a new dev-go/yaml package containing the 2.2.1 release
of https://gopkg.in/yaml.v2.

Also add dev-go/check with https://gopkg.in/check.v1 at
20d25e28, as this package is required by the yaml package's
tests.

BUG= chromium:885007 
TEST=built dev-go/{check,yaml} with FEATURES=test

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

[add] https://crrev.com/cf4ec05b94d19bd94bba17c6bedf20a80cfc6c68/dev-go/check/check-0.0.1.ebuild
[add] https://crrev.com/cf4ec05b94d19bd94bba17c6bedf20a80cfc6c68/dev-go/yaml/Manifest
[add] https://crrev.com/cf4ec05b94d19bd94bba17c6bedf20a80cfc6c68/dev-go/yaml/yaml-2.2.1.ebuild
[add] https://crrev.com/cf4ec05b94d19bd94bba17c6bedf20a80cfc6c68/dev-go/check/Manifest

I think I've finished writing the code for this (sorry, I got sidetracked) and am sending it for review now.

local_test_runner will read YAML files from /usr/local/etc/autotest-capability and determine each capability's state using logic that (hopefully) matches that of the existing Python code.

These capabilities will be exposed to tests as software features with "autotest-capability:" prefixes. (That prefix is long but just using "autotest:" feels ambiguous to me.)

When registering a test, you can use something like this:

testing.AddTest(&testing.Test{
    ...
    SoftwareDeps: []string{
        "autotest-capability:hw_dec_h264_1080_60",
        "autotest-capability:usb_camera",
        "chrome_login",
    },
    ...
})

The test will then be skipped on any DUTs that don't possess the hw_dec_h264_1080_60 and usb_camera capabilities and the chrome_login Tast software dependency (as described at go/tast-deps).

Note that you won't be able to check capabilities from within the test while it's running. Will that be a problem? I have a strong preference for keeping run/don't-run dependencies outside of tests since it's more of a scheduling decision.

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

commit 9cc54202a87419505a5372e81bf754f06a89fe28
Author: Daniel Erat <derat@chromium.org>
Date: Tue Sep 25 22:48:11 2018

tast-local-test-runner: Add dependency on dev-go/yaml.

This is needed to read autotest-dependency files.

BUG= chromium:885007 
TEST=emerged it

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

[modify] https://crrev.com/9cc54202a87419505a5372e81bf754f06a89fe28/chromeos-base/tast-local-test-runner/tast-local-test-runner-9999.ebuild

Sorry for delay reply.

> Note that you won't be able to check capabilities from within the test while it's running. Will that be a problem? I have a strong preference for keeping run/don't-run dependencies outside of tests since it's more of a scheduling decision.

I have two concerns on this point.
(1) We have one control file that includes video streams.
https://chromium.googlesource.com/chromiumos/third_party/autotest/+/master/client/site_tests/video_VDAPerf/control#32
Each of them has a required capability. If a device doesn't have it, just skip the stream.
This is an effort to reduce autotest scheduler load. If you don't take care much the load, I think we can split them of course.

(2) We have video_VideoCapability, test to verify capability settings matches the dynamic capability detector (avtest_label_detect).
In the test, we need to build capabilities from yaml files.
https://chromium.googlesource.com/chromiumos/third_party/autotest/+/master/client/site_tests/video_VideoCapability/video_VideoCapability.py

We indeed promise we will never skip inside of test by reading the yaml files.

What do you think?

> This is an effort to reduce autotest scheduler load.
> If you don't take care much the load, I think we can split them of course.

Autotest just schedules a single job to run Tast (roughly; see go/tast-infra), and Tast itself doesn't have any overhead from each additional test. I assume that your tests will need to log into Chrome before playing videos, though, which incurs an additional 6-7 second cost and some potential flakiness per test (because Chrome login seems to be less reliable than I'd like).

We have an increasing number of tests that need to be logged into Chrome but that don't care much about the environment they're in beyond that. I wonder if there's some way we can speed these tests up in Tast without introducing much additional complexity. I'll think about this some more.

The control file you linked to looks like it just has 8 video streams, so I think it's fine to have a separate test for each for the time being.

> (2) We have video_VideoCapability, test to verify capability
> settings matches the dynamic capability detector (avtest_label_detect).

To make sure I understand the relationship between autotest-capability and avtest_label_detect, is the following correct?

a) avtest_label_detect is a program that cros_label.py runs on lab DUTs to determine which Autotest labels should be assigned to them. These labels are used later to determine where to schedule tests.

b) autotest-capability is a mostly build-time description of the device's capabilities that's included in system images. While they're running, video tests use device_capability.py to confirm that the DUT has the capabilities that they need. If it doesn't, they fail with TestNAError.

So if I understand correctly, autotest-capability is currently only used to verify that DUTs have correct Autotest labels assigned while tests are running. If that's right, then I think that you don't need to port the video_VideoCapability test to Tast. Rather, autotest-capability will become the source of truth, and Tast will use it to determine whether each test should be run or skipped on a given DUT.

Re: (1), sounds good.

Re: (2),
avtest_label_detect traditionally have been used to attach labels by cros_label.py and then autotest-scheduler.
We changed the way due to their drawbacks and replace it with autotest-capability.

We would have to verify capabilities specified by yaml files, e.g. wrong capabilities by human mistake.
At that time, we can utilize avtest_label_detect. avtest_label_detect is also an installed command line in test image.
So we can run it and check autotest-capability's result matches avtest_label_detect's result.
If our specification is wrong, we can fix the yaml files. If avtest_label_detect's results are wrong, we must figure out the reason, e.g., device issue on lab and a general driver issue.

We can evaluate both results but we should trust static settings (i.e. autotest-capability), because it should follow a driver/device specification if there is no human mistake. So it should be used for scheduling tests.
As another bonus of video_VideoCapability, we can catch an appearance of a device whose specification we don't provide in autotest-capability.

In summary, I think the point is how we can make sure tests are scheduled for a device following driver/device specification.

Thanks

Thanks for the further explanation.

Hmm. So maybe I should update https://crrev.com/c/1240099 to move the autocaps package to chromiumos/tast/autocaps. Then you could add a video.VideoCapability (or whatever) Tast test that runs avtest_label_detect and also uses the autocaps package and ensures that the results match.

The downside is that we'd be running the go autotest-capability code twice, once before running tests and again within video.VideoCapability. It'll be fast, though, so that probably doesn't really matter.

Does that sound reasonable to you?

(I don't want to directly expose the DUT's features/capabilities to tests, since that encourages test authors to check dependencies at runtime instead of declaring their deps.)

> Hmm. So maybe I should update https://crrev.com/c/1240099 to move the autocaps package to chromiumos/tast/autocaps. Then you could add a video.VideoCapability (or whatever) Tast test that runs avtest_label_detect and also uses the autocaps package and ensures that the results match.
> 
> The downside is that we'd be running the go autotest-capability code twice, once before running tests and again within video.VideoCapability. It'll be fast, though, so that probably doesn't really matter.
> 
> Does that sound reasonable to you?

Yes. There must be only video_VideoCapability to need to know capabilities at run time. So I think this is not cost so much.

> (I don't want to directly expose the DUT's features/capabilities to tests, since that encourages test authors to check dependencies at runtime instead of declaring their deps.)

I agree with you; I inform Chrome OS Media team to not do like that in tast, and also monitor in migrating/developing media tests in tast.

Thanks.

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

commit 9639ae9df38edefee9072a752c8e2c35e23ae656
Author: Daniel Erat <derat@chromium.org>
Date: Fri Sep 28 02:44:30 2018

tast: Add autocaps package for autotest-capability.

Add a new autocaps package that local_test_runner will use
to parse autotest-capability files in
/usr/local/etc/autotest-capability. A later change will make
local_test_runner call this code when determining the DUT's
features.

The new package is at chromiumos/tast/caps instead of being
nested under the runner package since it will also need to
be used directly by a single video test that verifies that
the DUT's stated capabilities match those detected by
avtest_label_detect.

BUG= chromium:885007 
TEST=added unit test
CQ-DEPEND=Id9ff05f6179260ba2f9648d7c55287beb39e131e

Change-Id: I684298cb69ac278198962b2cb11e070e183b5b14
Reviewed-on: https://chromium-review.googlesource.com/1240099
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

[add] https://crrev.com/9639ae9df38edefee9072a752c8e2c35e23ae656/src/chromiumos/tast/autocaps/detect.go
[add] https://crrev.com/9639ae9df38edefee9072a752c8e2c35e23ae656/src/chromiumos/tast/autocaps/caps_test.go
[add] https://crrev.com/9639ae9df38edefee9072a752c8e2c35e23ae656/src/chromiumos/tast/autocaps/caps.go

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

commit 66b7895f3b95d9a5723b859c23cb78c7f22202ae
Author: Daniel Erat <derat@chromium.org>
Date: Fri Sep 28 13:26:05 2018

tast-cmd, tast-local-tests-cros: Add dev-go/yaml dep.

Make the tast-cmd and tast-local-tests-cros packages depend
on dev-go/yaml. tast-cmd will link and test code that
depends on this, and tast-local-tests-cros will soon need it
in tests.

BUG= chromium:885007 
TEST=emerged both packages

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

[modify] https://crrev.com/66b7895f3b95d9a5723b859c23cb78c7f22202ae/chromeos-base/tast-local-tests-cros/tast-local-tests-cros-9999.ebuild
[modify] https://crrev.com/66b7895f3b95d9a5723b859c23cb78c7f22202ae/chromeos-base/tast-cmd/tast-cmd-9999.ebuild

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

commit df04ea08f4b7b26f92f8c4fac8f8c72bde7978c8
Author: Daniel Erat <derat@chromium.org>
Date: Fri Sep 28 13:26:06 2018

tast: Support autotest-capability dependencies.

Update local_test_runner to read autotest-capability YAML
files from /usr/local/etc/autotest-capability and generate
additional features with names like
"autotest-capability:hw_dec_jpeg".

BUG= chromium:885007 
TEST=added unit test; also ran "tast run" with
     -buildlocalrunner against a guado device and saw the
     following new features (which include kepler
     capabilities):
    autotest-capability:hw_dec_h264_1080_30
    autotest-capability:hw_dec_h264_1080_60
    autotest-capability:hw_dec_h264_2160_30
    autotest-capability:hw_dec_jpeg
    autotest-capability:hw_dec_vp8_1080_30
    autotest-capability:hw_dec_vp9_1080_30
    autotest-capability:hw_dec_vp9_1080_60
    autotest-capability:hw_enc_h264_1080_30
    autotest-capability:hw_enc_vp8_1080_30
CQ-DEPEND=I95822a2678ef8729dd28c2f9ea53addf97363ea8

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

[modify] https://crrev.com/df04ea08f4b7b26f92f8c4fac8f8c72bde7978c8/docs/test_dependencies.md
[modify] https://crrev.com/df04ea08f4b7b26f92f8c4fac8f8c72bde7978c8/src/chromiumos/cmd/tast/run/deps.go
[modify] https://crrev.com/df04ea08f4b7b26f92f8c4fac8f8c72bde7978c8/src/chromiumos/tast/runner/args.go
[modify] https://crrev.com/df04ea08f4b7b26f92f8c4fac8f8c72bde7978c8/src/chromiumos/cmd/local_test_runner/main.go
[modify] https://crrev.com/df04ea08f4b7b26f92f8c4fac8f8c72bde7978c8/src/chromiumos/tast/runner/features_test.go
[modify] https://crrev.com/df04ea08f4b7b26f92f8c4fac8f8c72bde7978c8/src/chromiumos/tast/runner/features.go