Skip to content

Commit

Permalink
Add grace period option to GPU pixel tests
Browse files Browse the repository at this point in the history
Adds the option for GPU pixel tests to define a grace period, during
which failures will not be surfaced on the waterfall bots. This is to
help prevent newly added tests from turning the bots red due to a
bunch of untriaged images showing up after a test is first added. The
grace period gives the test author a bit of time to triage any new
images before the test starts failing.

Bug: 1008467
Change-Id: I14c219f86ababe96db4d95f18a4f7ff9bf5b469f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1829465
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Commit-Queue: Yuly Novikov <ynovikov@chromium.org>
Auto-Submit: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Yuly Novikov <ynovikov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700892}
  • Loading branch information
Brian Sheedy authored and Commit Bot committed Sep 27, 2019
1 parent fe543d9 commit 5a88cc7
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 3 deletions.
27 changes: 25 additions & 2 deletions content/test/gpu/gpu_tests/pixel_integration_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

from datetime import date
import json
import logging
import os
Expand Down Expand Up @@ -427,8 +428,30 @@ def _UploadTestResultToSkiaGold(self, image_name, screenshot,
SKIA_GOLD_INSTANCE, image_name))
logging.error(
'Approved images for %s in Gold: %s', image_name, gold_images)
if not parsed_options.no_skia_gold_failure:
raise Exception('goldctl command failed')
if self._ShouldReportGoldFailure(page):
raise Exception('goldctl command failed, see above for details')

def _ShouldReportGoldFailure(self, page):
"""Determines if a Gold failure should actually be surfaced.
Args:
page: The GPU PixelTestPage object for the test.
Returns:
True if the failure should be surfaced, i.e. the test should fail,
otherwise False.
"""
parsed_options = self.GetParsedCommandLineOptions()
# Don't surface if we're explicitly told not to.
if parsed_options.no_skia_gold_failure:
return False
# Don't surface if the test was recently added and we're still within its
# grace period. However, fail if we're on a trybot so that as many images
# can be triaged as possible before a new test is committed.
if (page.grace_period_end and date.today() <= page.grace_period_end and
not parsed_options.review_patch_issue):
return False
return True

def _ValidateScreenshotSamplesWithSkiaGold(self, tab, page, screenshot,
device_pixel_ratio,
Expand Down
8 changes: 7 additions & 1 deletion content/test/gpu/gpu_tests/pixel_test_pages.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class PixelTestPage(object):
def __init__(self, url, name, test_rect, revision,
tolerance=2, browser_args=None, expected_colors=None,
gpu_process_disabled=False, optional_action=None,
other_args=None):
other_args=None, grace_period_end=None):
super(PixelTestPage, self).__init__()
self.url = url
self.name = name
Expand Down Expand Up @@ -66,6 +66,12 @@ def __init__(self, url, name, test_rect, revision,
# VideoPathTraceTest and OverlayModeTest support the following boolean
# arguments: expect_yuy2, zero_copy, video_is_rotated, and no_overlay.
self.other_args = other_args
# This allows a newly added test to be exempted from failures for a
# (hopefully) short period after being added. This is so that any slightly
# different but valid images that get produced by the waterfall bots can
# be triaged without turning the bots red.
# This should be a datetime.date object.
self.grace_period_end = grace_period_end

def CopyWithNewBrowserArgsAndSuffix(self, browser_args, suffix):
return PixelTestPage(
Expand Down
27 changes: 27 additions & 0 deletions docs/gpu/gpu_testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -522,9 +522,36 @@ pixel test failures are actual failures or need to be rebaselined." step.

[pixel wrangling triage]: pixel_wrangling.md#How-to-Keep-the-Bots-Green

If you are adding a new pixel test, it is beneficial to set the
`grace_period_end` argument in the test's definition. This will allow the test
to run for a period without actually failing on the waterfall bots, giving you
some time to triage any additional images that show up on them. This helps
prevent new tests from making the bots red because they're producing slightly
different but valid images from the ones triaged while the CL was in review.
Example:

```
from datetime import date
...
PixelTestPage(
'foo_pixel_test.html',
...
grace_period_end=date(2020, 1, 1)
)
```

You should typically set the grace period to end 1-2 days after the the CL will
land.

Once your CL passes the CQ, you should be mostly good to go, although you should
keep an eye on the waterfall bots for a short period after your CL lands in case
any configurations not covered by the CQ need to have images approved, as well.
All untriaged images for your test can be found by substituting your test name
into:

`https://chrome-gpu-gold.skia.org/search?query=name%3D<test name>`

## Stamping out Flakiness

Expand Down

0 comments on commit 5a88cc7

Please sign in to comment.