+phajdan.jr

+jbudorick

cc-ing current trooper.

Something seems pretty broken in this area. I can't imagine that tryjobs which break any IsolatedScriptTest type steps are broken.

Not sure whether IsolatedScriptTest obeys all of the contracts of recipes in handling retries without the patch.

This tryjob:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/423967

contained failing steps but the overall build was said to have succeeded.

I'll take this for now, as martiniss@ and I suspect it may be related to my merge change from last week. My guess would be that this is specific to steps that don't emit any test failures.

... though this could also be some of the recent result validation changes in chromium_tests/steps.py. Still looking.

Reproduced in simulation tests w/ chromium_trybot only: https://chromium-review.googlesource.com/c/479932/

Investigating + working on a fix.

Fix uploaded: https://chromium-review.googlesource.com/c/479932/

This has multiple failing steps - why is the whole thing marked as passing? What logic is ignoring the failing steps?

So what's happening here is:

-- outside of the recipe
 1) test in question fails immediately, with no JSON output
 2) the standard isolated_script merge emits an empty JSON given that lack of JSON output
 3) collect_cmd exits with a nonzero exit code

-- back in the recipe
 4) SwarmingIsolatedScriptTest does results validation (https://codesearch.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_tests/steps.py?rcl=e1a82b354acb97dc2156a4eeb20b54886fac18fd&l=1502) and winds up with valid results and no failures (because there are no results)
 5) (speculative) api.test_utils.run_tests catches the StepFailure and adds it to its list of failed_tests
 6) api.test_utils.run_tests_with_patch doesn't use the list of failed_tests returned by api.test_utils.run_tests. Instead, it does its own failure detection by checking (1) if valid is True and (2) if failures are empty. Notably, this ignores the exit code.

The patchset I posted changes the validate_task_results logic in SwarmingIsolatedScriptTest to make sure that tests in such scenarios are marked as invalid. I think that changing the failure detection in api.test_utils.run_tests_with_patch could also be a plausible if broader fix.

Thanks John for getting to the bottom of this.

If this is a recent regression, could you link this bug to the other one where the original change was made? Thanks.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/20a111faa801b8cce14584c3cf9c9e2d34bbb59d

commit 20a111faa801b8cce14584c3cf9c9e2d34bbb59d
Author: John Budorick <jbudorick@chromium.org>
Date: Thu Apr 20 13:21:14 2017

Mark results as invalid when retcode is nonzero and no results are returned.

Bug:  712408 
Change-Id: I369186e45f0e77c3f0caef57bc7c7ca08514b1a4
Reviewed-on: https://chromium-review.googlesource.com/479932
Commit-Queue: John Budorick <jbudorick@chromium.org>
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>

[add] https://crrev.com/20a111faa801b8cce14584c3cf9c9e2d34bbb59d/scripts/slave/recipes/chromium.expected/dynamic_swarmed_isolated_script_test_failure_no_result_json.json
[add] https://crrev.com/20a111faa801b8cce14584c3cf9c9e2d34bbb59d/scripts/slave/recipes/chromium_trybot.expected/dynamic_swarmed_isolated_script_test_failure_no_result_json.json
[modify] https://crrev.com/20a111faa801b8cce14584c3cf9c9e2d34bbb59d/scripts/slave/recipes/chromium_trybot.py
[modify] https://crrev.com/20a111faa801b8cce14584c3cf9c9e2d34bbb59d/scripts/slave/recipes/chromium.py
[modify] https://crrev.com/20a111faa801b8cce14584c3cf9c9e2d34bbb59d/scripts/slave/recipe_modules/chromium_tests/steps.py

#11: This was likely a regression I introduced with the isolated_script merge rework back in https://chromium-review.googlesource.com/c/441391/.