New issue
Advanced search Search tips

Issue 885194 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 20
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 888734



Sign in to add a comment

test_installer returns invalid test results on test failure.

Project Member Reported by erikc...@chromium.org, Sep 18

Issue description

This causes the CQ to incorrectly interpret the test failure.

https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8935114539403456640/+/steps/test_installer__with_patch_/0/stdout

======================================================================
FAIL: ChromeUserLevelUpdate: no_pv -> install_chrome_user -> chrome_user_installed_not_inuse -> update_chrome_user -> chrome_user_updated_not_inuse -> test_chrome_with_chromedriver_user -> chrome_user_installed_not_inuse -> uninstall_chrome_user -> clean
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\b\swarming\w\ir\cache\builder\src\chrome\test\mini_installer\test_installer.py", line 160, in runTest
    self._VerifyState(state)
  File "C:\b\swarming\w\ir\cache\builder\src\chrome\test\mini_installer\test_installer.py", line 194, in _VerifyState
    raise AssertionError("In state '%s', %s" % (state, e))
AssertionError: In state 'chrome_user_updated_not_inuse', Value opv of registry key HKEY_CURRENT_USER\Software\Chromium exists with value 71.0.3555.0
----------------------------------------------------------------------
Ran 8 tests in 97.296s
FAILED (failures=1)
step returned non-zero exit code: 1
 
Cc: gbeaty@chromium.org liaoyuke@chromium.org st...@chromium.org jbudorick@chromium.org martiniss@chromium.org
Owner: erikc...@chromium.org
Status: Assigned (was: Untriaged)
The root problem here is that test_installer is run as a local Python test [specifically, MiniInstallerTest]:

https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_tests/steps.py?type=cs&q=MiniInstallerTest&sq=package:chromium&g=0&l=2411 

And test failures are raised as Python exceptions:
https://cs.chromium.org/chromium/src/chrome/test/mini_installer/test_installer.py?type=cs&q=test_installer.py&sq=package:chromium&g=0&l=194

Instead of correctly propagating up as failures by writing to the parameter passed in "--write-full-results-to".

The solution is to modify test_installer.py to not throw exceptions, but instead to write out results.
Actually, test_installer.py appears to be correctly writing a JSON to the output file. Not sure why recipe is throwing an invalid test results. 

I've cut some of the test outputs to make it shorter, but this is the output if an exception is thrown during the test.

"""
{
  "tests": {
    "__main__": {
      "InstallerTest": {
        "ChromeSystemLevel": {
          "expected": "PASS", 
          "actual": "FAIL", 
          "is_unexpected": true
        }, 

        <removed for brevity>

        "MigrateMultiStrandedBinariesOnUpdate": {
          "expected": "PASS", 
          "actual": "FAIL", 
          "is_unexpected": true
        }
      }
    }
  }, 
  "interrupted": false, 
  "path_delimiter": ".", 
  "version": 3, 
  "seconds_since_epoch": 1537304440.948, 
  "num_failures_by_type": {
    "FAIL": 8, 
    "PASS": 0
  }
}
"""
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 20

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

commit 503174a3e1e439c6ab0b18d263980fd18a3d2855
Author: erikchen <erikchen@chromium.org>
Date: Thu Sep 20 21:01:30 2018

Fix PythonBasedTest to correctly handle failures and invalid results.

Previously, invalid results were being marked as failures, and failures were
being marked as invalid results.

This CL makes the following changes:
  * Simplifies the control flow of PythonBasedTest.Run().
    * Fixes a bug where self.failures() was being called before setting
      self._test_runs[suffix]. The implementation of the former relies on the
      latter.
    * Correctly sets INVALID_RESULTS_MAGIC on invalid results.
    * Adds tests.

  * Modifies TestUtilsTestApi to allow tests to easily construct invalid
    results. TestUtilsTestApi.test_results() is supposed to be a wrapper for a
    JSON string, which will be converted to a TestResults() by
    TestResultsOutputPlaceholder. The interface for test_results() required a
    TestResults() object, which would then be serialized to JSON. This meant
    that tests could not easily pass arbitrary JSON results.

  * Modifies TestResults to never use a default value for self.raw. This
    caused invalid test results to actually appear as failure test results.

Change-Id: I71d64dafbdcccd72def9d1e8ea7d93a368d57b5a
Bug:  885194 
Reviewed-on: https://chromium-review.googlesource.com/1234173
Reviewed-by: John Budorick <jbudorick@chromium.org>
Reviewed-by: Stephen Martinis <martiniss@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>

[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipes/chromium.expected/dynamic_swarmed_isolated_script_test_failure_no_result_json.json
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipes/chromium_trybot.py
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipe_modules/chromium_tests/tests/steps/mini_installer_test.py
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipe_modules/test_utils/util.py
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_isolated_script_test.expected/chartjson_no_results_failure.json
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_isolated_chartjson_test_harness_failure.json
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipe_modules/test_utils/test_api.py
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_gtest_test.py
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/README.recipes.md
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipe_modules/chromium_tests/steps.py
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipes/chromium_trybot.expected/dynamic_swarmed_isolated_script_test_failure_no_result_json.json
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipes/boringssl.py
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipe_modules/chromium_tests/tests/api/run_tests_on_tryserver.py
[modify] https://crrev.com/503174a3e1e439c6ab0b18d263980fd18a3d2855/scripts/slave/recipes/findit/chromium/test.py

Status: Fixed (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/135aad0048df9151337bea3bd39b4c8d138f3d1d

commit 135aad0048df9151337bea3bd39b4c8d138f3d1d
Author: Oleh Prypin <oprypin@webrtc.org>
Date: Fri Sep 21 13:20:54 2018

Make webrtc_perf_tests output an empty result output.json

to satisfy a stricter check introduced in
https://chromium.googlesource.com/chromium/tools/build/+/503174a3e1e439c6ab0b18d263980fd18a3d2855

The file is supposed to contain actual gtest results, so having an
empty one is a workaround, but this just returns things to the way
they were.

TBR: phoglund@webrtc.org
No-Try: True
Bug:  webrtc:9767 ,  chromium:885194 
Change-Id: I693cc2df9dfcafd7b728deb9efd445d8fe2c4edf
Reviewed-on: https://webrtc-review.googlesource.com/101301
Commit-Queue: Oleh Prypin <oprypin@webrtc.org>
Reviewed-by: Yves Gerey <yvesg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24783}
[modify] https://crrev.com/135aad0048df9151337bea3bd39b4c8d138f3d1d/test/test_main.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/97cbd3a1ffe335e92074f69a043a264b571402ba

commit 97cbd3a1ffe335e92074f69a043a264b571402ba
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Mon Sep 24 10:30:25 2018

Roll src/third_party/webrtc 0ad0c27a0bce..645512ba5966 (5 commits)

https://webrtc.googlesource.com/src.git/+log/0ad0c27a0bce..645512ba5966


git log 0ad0c27a0bce..645512ba5966 --date=short --no-merges --format='%ad %ae %s'
2018-09-24 asapersson@webrtc.org Add field trial to allow always using max layers.
2018-09-21 oprypin@webrtc.org Make Python-based performance tests output an empty result output.json
2018-09-21 oprypin@webrtc.org Make webrtc_perf_tests output an empty result output.json
2018-09-21 yvesg@webrtc.org Output 0 instead of NaN in perftest-output.json
2018-09-20 borenet@google.com Add extra logging to roll_deps.py


Created with:
  gclient setdep -r src/third_party/webrtc@645512ba5966

The AutoRoll server is located here: https://autoroll.skia.org/r/webrtc-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux_chromium_archive_rel_ng;luci.chromium.try:mac_chromium_archive_rel_ng

BUG=chromium:none,chromium:885194
TBR=webrtc-chromium-sheriffs-robots@google.com

Change-Id: I703605d9450e1e3ed49b3d68e57d67757612db54
Reviewed-on: https://chromium-review.googlesource.com/1239484
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#593499}
[modify] https://crrev.com/97cbd3a1ffe335e92074f69a043a264b571402ba/DEPS

Blocking: 888734
Labels: Infra-Platform-Test

Sign in to add a comment