New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 904019 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression

Blocking:
issue 835690



Sign in to add a comment

Catapult roll is blocked by telemetry_gpu_unittests failure

Project Member Reported by nedngu...@google.com, Nov 10

Issue description

I 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.

 
Blocking: 835690
Labels: -Type-Bug -Pri-3 Pri-1 Type-Bug-Regression
Those unit tests aren't wired up to use the new functionality, so this sounds like an accidental regression. https://chromium-review.googlesource.com/1171876 should probably be reverted unless Dirk can look into this. I'm available for consultation.

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
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).
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?

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.


#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.
Owner: nedngu...@google.com
Status: Started (was: Untriaged)
I am TBR'ing some fixes to unblock the roll..
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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.
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. 
Yeah, I think that if you *do* have expectations, then if a test calls .skip() that should be treated as an unexpected failure.
Agree with the assessment in #14.

Sign in to add a comment