Issue metadata
Sign in to add a comment
|
Catapult roll is blocked by telemetry_gpu_unittests failure |
||||||||||||||||||||||
Issue descriptionI think this has to do with Dirk's CL in https://chromium-review.googlesource.com/c/catapult/+/1171876 Log: https://chromium-swarm.appspot.com/task?id=41134c260a4fc010&refresh=10 AssertionError: Lists differ: [u'unittest_data.integration_t... != ['unittest_data.integration_te... First differing element 1: u'unittest_data.integration_tests.SimpleTest.expected_skip' 'unittest_data.integration_tests.SimpleTest.unexpected_failure' First list contains 1 additional elements. First extra element 2: u'unittest_data.integration_tests.SimpleTest.unexpected_failure' - [u'unittest_data.integration_tests.SimpleTest.unexpected_error', ? - + ['unittest_data.integration_tests.SimpleTest.unexpected_error', - u'unittest_data.integration_tests.SimpleTest.expected_skip', - u'unittest_data.integration_tests.SimpleTest.unexpected_failure'] ? - + 'unittest_data.integration_tests.SimpleTest.unexpected_failure'] 37 tests passed in 26.7s, 0 skipped, 1 failure.
,
Nov 10
Looks like what's going on is Dirk's CL make telemetry gpu tests's skip no longer be skipped. Lemme dig more into it
,
Nov 10
Basically Dirk's CL change the expectation of test foo/bar from "SKIP" to "PASS" even if test foo/bar invoke self.skipTest(...). This is a bit different to how it used to be, given now that all test has its expectation clearly specified in the expectation file (and probably has expectation set as "PASS" if not specified).
,
Nov 10
Is there any middle ground which allows both the old behavior for old tests and the new behavior for new ones that use the new test expectations mechanism?
,
Nov 10
For minimal reproducible, https://chromium-review.googlesource.com/c/catapult/+/1330927 shows a unittest that is currently failing & will pass if we revert Dirk's CL.
,
Nov 10
#4: I think the one way is to add some temporary flag like "--skipTest-cause-SKIP-expectation" & turn that flag on for GPU tests. Once Rakib converted GPU tests to use expectation file (issue 698902), when then remove that flag.
,
Nov 10
I am TBR'ing some fixes to unblock the roll..
,
Nov 10
The following revision refers to this bug: https://chromium.googlesource.com/catapult/+/f2c3502f7788368a5441e764592d4c47a0bb2582 commit f2c3502f7788368a5441e764592d4c47a0bb2582 Author: Nghia Nguyen <nednguyen@google.com> Date: Sat Nov 10 04:34:42 2018 [browser_test_runner] Ensure that skipped tests have skipped expectation if typ expectation file is not used Bug: chromium:904019 Change-Id: I662c75d80afee6b01a88acd1cd602e821ba8f36d TBR=kbr@chromium.org, rmhasan@google.com Reviewed-on: https://chromium-review.googlesource.com/c/1330931 Reviewed-by: Ned Nguyen <nednguyen@google.com> Commit-Queue: Ned Nguyen <nednguyen@google.com> [modify] https://crrev.com/f2c3502f7788368a5441e764592d4c47a0bb2582/telemetry/telemetry/testing/run_browser_tests.py
,
Nov 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a1e0c09bcfef3dcc195179ff082327b9146c5bd8 commit a1e0c09bcfef3dcc195179ff082327b9146c5bd8 Author: Ned Nguyen <nednguyen@google.com> Date: Sat Nov 10 06:17:18 2018 Make sure that GpuIntegrationTestUnittest test results assertions is not sensitive to order of test TBR=kbr@chromium.org, rmhasan@google.com Bug: chromium:904019 Change-Id: I6f6bf317dc25ee9d3bcf3c1c2163a3b5afff3086 Reviewed-on: https://chromium-review.googlesource.com/c/1330990 Reviewed-by: Ned Nguyen <nednguyen@google.com> Commit-Queue: Ned Nguyen <nednguyen@google.com> Cr-Commit-Position: refs/heads/master@{#607110} [modify] https://crrev.com/a1e0c09bcfef3dcc195179ff082327b9146c5bd8/content/test/gpu/gpu_tests/gpu_integration_test_unittest.py
,
Nov 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/33c10c78715457aac9937c0a5b71b477233cc9da commit 33c10c78715457aac9937c0a5b71b477233cc9da Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Date: Sat Nov 10 09:22:50 2018 Roll src/third_party/catapult 5d5091665700..f2c3502f7788 (9 commits) https://chromium.googlesource.com/catapult.git/+log/5d5091665700..f2c3502f7788 git log 5d5091665700..f2c3502f7788 --date=short --no-merges --format='%ad %ae %s' 2018-11-10 nednguyen@google.com [browser_test_runner] Ensure that skipped tests have skipped expectation if typ expectation file is not used 2018-11-09 benjhayden@chromium.org Add BatchIterator for v2spa 2018-11-09 benjhayden@chromium.org Merge test suite descriptors in v2spa 2018-11-09 benjhayden@chromium.org Add RecommendedOptions to v2spa 2018-11-09 benjhayden@chromium.org Support uploading to dev_appserver.py 2018-11-09 benjhayden@chromium.org Generalize complex test cases in Descriptor. 2018-11-09 benjhayden@chromium.org Expose annotations via /api/timeseries2 2018-11-09 benjhayden@chromium.org Add tag-filter for v2spa 2018-11-08 dpranke@chromium.org Integrate test expectation-parsing code into TYP. Created with: gclient setdep -r src/third_party/catapult@f2c3502f7788 The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-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:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel BUG= chromium:904019 ,chromium:898552,chromium:900218, chromium:835690 TBR=sullivan@chromium.org Change-Id: Ic8d2d5b5c618bef91ecd2c6051a9e85ab340dab9 Reviewed-on: https://chromium-review.googlesource.com/c/1330771 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@{#607117} [modify] https://crrev.com/33c10c78715457aac9937c0a5b71b477233cc9da/DEPS
,
Nov 12
,
Nov 16
I think this was a bug that crept in when I split up my CLs ... When we aren't using expectations files, you have to decide how to treat unittest.case.TestCase.skip() - is that an expected failure, or an unexpected failure (there's no way to distinguish in the API). I think initially I treated this as an expected failure, but I think when I was splitting the CLs for landing, for some reason I decided this should be unexpected instead, and I think that probably led to the failures you saw? This was probably a mistake. If need be, we can fix this in TYP and back out your change. Let me know if you want that.
,
Nov 16
I think the current typ's distinction between SKIP by expectation vs SKIP in actual is closer to what we want in the final state. So given then, I think the work around in #9 is ok.
,
Nov 16
Yeah, I think that if you *do* have expectations, then if a test calls .skip() that should be treated as an unexpected failure.
,
Nov 16
Agree with the assessment in #14. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by kbr@chromium.org
, Nov 10Labels: -Type-Bug -Pri-3 Pri-1 Type-Bug-Regression