The problem is that during host creation, servo host arguments could not be inferred.
Need to understand a little more about the setup to understand why this used to work before my work to extract servo arguments from HostInfo.

The autoserv logs say:

09/05 13:23:44.117 DEBUG|        servo_host:0842| No servo_args provided, and failed to find overrides.

Which is not surprising since no servo information is provided when running with test_that.
The default of <hostname>-servo.cros will also not be tried because the DUT was referred to via IP, not its hostname (100.90.26.147)

Mary: I need some more information about your setup to support this properly.

- Where is the DUT? Is it in the lab?
- How do you get the servo information for the DUT? (do you?)

- Do you have logs from an earlier successful run on a similar setup?

I suspect that this was working previously because even though you were using test_that, autoserv was somehow pinging cautotest/ for DUT information. This is wrong, and I've been changing things over the past year or so so that this doesn't happen.

We need to figure out what exactly your use case is, then I can advise on how to proceed.

Please reassign with more information.

The dut is at my workstation.
servo port is 9999

What's the command you're supposed to run?

Deleted: test_that_results_aus5hb.tar.gz 1.3 MB

test_that_results_aus5hb.tar.gz 1.3 MB Download

If I run test_that with 
--args="servo_host=localhost"
and make a change to server/hosts/servo_host.py, I can run test_that again


diff --git a/server/hosts/servo_host.py b/server/hosts/servo_host.py
index f94e6de57..d16546103 100644
--- a/server/hosts/servo_host.py
+++ b/server/hosts/servo_host.py
@@ -760,7 +760,7 @@ def _get_servo_args_for_host(dut_host):
 
     if info.board:
         servo_args[SERVO_BOARD_ATTR] = _map_afe_board_to_servo_board(info.board)
-    return servo_args
+    return servo_args if servo_args else None
 
 
 def _tweak_args_for_ssp_moblab(servo_args):


You should probably look into fixing servo_host.py _get_servo_args_for_host. I don't know if my fix is the correct way to do things

Re #10, thanks for getting your hands dirty :)

I think you're pretty close to the command that should work.

these tests do allow passing servo arguments as test args, so you want:
https://cs.corp.google.com/chromeos_public/src/third_party/autotest/files/server/site_tests/firmware_ECPowerButton/control?l=29

--args="servo_host=localhost servo_port=9903"

should do the right thing.

I think the problem is not in inferring the servo_args, but rather that the servo_args from commandline aren't passed in.
Looking...

And.. you were right on the mark: https://chromium-review.googlesource.com/c/chromiumos/third_party/autotest/+/1208893

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

commit 6f5f636484debf01738bbfd98634595ed371b070
Author: Prathmesh Prabhu <pprabhu@chromium.org>
Date: Sat Sep 08 01:36:57 2018

autotest: Require servo_host among servo attributes from host info

BUG= chromium:881006 
TEST=test_that -b soraka <dut_ip> \
         --autotest_dir ~/trunk/src/third_party/autotest/files/ \
         --args="servo_host=localhost servo_port=9903" \
         firmware_ECPowerButton

Change-Id: Ib150d8350d208f0eb35e24a435669a5ac357c412
Reviewed-on: https://chromium-review.googlesource.com/1208893
Commit-Ready: Prathmesh Prabhu <pprabhu@chromium.org>
Tested-by: Prathmesh Prabhu <pprabhu@chromium.org>
Reviewed-by: Mary Ruthven <mruthven@chromium.org>

[modify] https://crrev.com/6f5f636484debf01738bbfd98634595ed371b070/server/hosts/servo_host.py

faft setup pass after passing --argsargs="servo_host=localhost servo_port=9999"

test_that --autotest_dir ~/trunk/src/third_party/autotest/files/ --args="servo_host=localhost servo_port=9999" -b eve venkataraju-servov2-1.mtv f:.*firmware_FAFTSetup/control

Thanks for verifying.

Can we change the servo_host=localhost config back to default?  A bunch of us in the fw team have spent a bunch of time trying to debug problems with FAFT because of this change.  I don't think any of us were aware of this recent change.

pprabhu, please address the request of c#17

When running a test via test_that, the user must provide arguments for the test to use. This includes the servo host and servo port arguments, if the test uses servo explicitly (i.e., test needs servo for directly controlling the DUT).

I'd recommend against adding any defaults for these arguments, as that leads to weird "knowledge" about the test one needs to have in order to run the test. Instead, the test should check that the bare minimum arguments needed for execution were provided, and fail with a helpful message otherwise.

Something along the lines of: https://chromium-review.googlesource.com/c/chromiumos/third_party/autotest/+/1237316

OTOH, I can't add an error like this for all possible tests. Too many tests have simply copy-pasted the control file so that I have not idea how many of these tests actually need servo (vs, copied the servo argument extraction without actually needing to): http://shortn/_m9A4ZahrBH

Re-marking as Fixed because I do not intend to add global defaults for these arguments (for all tests).
For this particular test, I'm happy to land the example CL provided here.

What I don't understand is why it broke. Before if the tests used servo, they used localhost and port 9999 without the user having to specify it. It didnt't query cautotest or something strange like that.

The default "servo_host=localhost" is well-known for many years, since the time we wrote FAFT. Developers and partners get used of it, i.e. not passing any --args to run FAFT, like what the FAFT instruction states:
  http://www.chromium.org/for-testers/faft

I think our partners also mirror some similar instructions on their sites. I am afraid they will keep reporting bugs like this about failing to run FAFT locally.

Actually we can't assume developers/partners know if running a test needs "servo". For example, do you think if running "system_ColdBoot" needs the servo_host argument? and "platform_Powerwash"? People can't tell. So making it default helps. The test control file specifies the need of servo, i.e. ignoring the servo_host argument if the test doesn't need servo, like:

  host = hosts.create_host(machine, servo_args=servo_args)
vs
  host = hosts.create_host(machine)

https://chromium.googlesource.com/chromiumos/third_party/autotest/+/master/server/site_tests/system_ColdBoot/control#31

If you want to add some error check, probably do it on the servo_args = hosts.CrosHost.get_servo_arguments(args_dict).

Re #21: I agree that the breakage seems like a side-effect.
Unfortunately, this path did consult AFE, which is what I intentionally removed. The default seems to have died as collateral.

Again, adding the default back wouldn't be against my initial reason for this change (i.e., I could just say "whatever" and move on with life), which is likely what I'll have to do here after all.
But my experience so far tells me that defaults like this which allow people to run tests without knowing what they're doing lead to unmaintanable code and feature creep. (nobody knows who is using what defaults, the users don't know what they're doing...)

In this case, the default of localhost:9999 works because they're running local servod, but they're likely unaware of that.

I understand the need for a one-stop-it-just-works tool for partners etc, but test_that isn't designed to be that tool. Since test_that didn't start servod on your workstation for you, test_that has no business guessing where servod is running.

My pretty forceful 2 cents, while I work to add back the defaults I dislike... :(

Will work on this at P2 (ETA update next week).

Explicitly adds back defaults only for test_that local runs: 
https://chromium-review.googlesource.com/c/chromiumos/third_party/autotest/+/1247461

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

commit b3dd6c3aafeb2c379299d1f205833d5114baa74d
Author: Prathmesh Prabhu <pprabhu@chromium.org>
Date: Sat Sep 29 07:27:40 2018

test_that: Add default test args for backward compatibility

BUG= chromium:881006 
TEST=test_that <dut> firmware_FAFTSetup

Change-Id: I3dc8c277965c14b9e18261faae70659df8a7f0f2
Reviewed-on: https://chromium-review.googlesource.com/1247461
Commit-Ready: Prathmesh Prabhu <pprabhu@chromium.org>
Tested-by: Prathmesh Prabhu <pprabhu@chromium.org>
Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org>

[modify] https://crrev.com/b3dd6c3aafeb2c379299d1f205833d5114baa74d/site_utils/test_runner_utils.py
[modify] https://crrev.com/b3dd6c3aafeb2c379299d1f205833d5114baa74d/site_utils/test_runner_utils_unittest.py