Skip to content

Commit

Permalink
Set wpt-importer notification from opt-in to opt-out
Browse files Browse the repository at this point in the history
Bug: 1454853
Change-Id: I59e1c4927de0f79e4581d0cf593a1217eb038817
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4803074
Reviewed-by: Weizhong Xia <weizhong@google.com>
Reviewed-by: Jonathan Lee <jonathanjlee@google.com>
Commit-Queue: An Sung <ansung@google.com>
Cr-Commit-Position: refs/heads/main@{#1187581}
  • Loading branch information
dev-ansung authored and Chromium LUCI CQ committed Aug 24, 2023
1 parent d82a9ee commit 183d2ac
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 19 deletions.
34 changes: 22 additions & 12 deletions docs/testing/web_platform_tests.md
Original file line number Diff line number Diff line change
Expand Up @@ -208,22 +208,20 @@ For maintainers:

### New failure notifications

Test owners can elect to have the importer automatically file bugs against a
component when imported changes introduce failures. This includes new tests that
fail in Chromium, as well as new failures introduced to an existing test. To
opt-in to this functionality, create an `DIR_METADATA` file in the appropriate
`external/wpt/` subdirectory that contains at least `wpt.notify` and
`monorail.component` fields. For example, `external/wpt/css/css-grid/DIR_METADATA`
looks like:
The importer automatically file bugs against a component when imported changes
introduce failures as long as test owners did not choose to opt-out the failure
notification mechanism. This includes new tests that fail in Chromium, as well
as new failures introduced to an existing test. Test owners are encouraged to
create an `DIR_METADATA` file in the appropriate `external/wpt/` subdirectory
that contains at least `monorail.component` fields, which will be used by the
importer to file the bugs.
For example, `external/wpt/css/css-grid/DIR_METADATA` looks like:

```
monorail {
component: "Blink>Layout>Grid"
}
team_email: "layout-dev@chromium.org"
wpt {
notify: YES
}
```

When tests under `external/wpt/css/css-grid/` newly fail in a WPT import, the
Expand All @@ -236,8 +234,20 @@ The importer will also copy `layout-dev@chromium.org` (the `team_email`) and any
Failing tests are grouped according to the most specific `DIR_METADATA` that
they roll up to.

Note that we are considering making the notifications opt-out instead of
opt-in: see https://crbug.com/1454853
To opt-out of this notification, add `wpt.notify` field set to `NO` to the
corresponding `DIR_METADATA`.
For example, the following `DIR_METADATA` will suppress notification from tests
under the located directory:

```
monorail {
component: "Blink>Layout>Grid"
}
team_email: "layout-dev@chromium.org"
wpt {
notify: NO
}
```

### Skipped tests (and how to re-enable them)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,8 @@ def read_dir_metadata(self, path: str) -> Optional[WPTDirMetadata]:
return None

# The value of `notify` is one of ['TRINARY_UNSPECIFIED', 'YES', 'NO'].
# Assume that users opt out by default; return True only when notify is
# 'YES'.
#
# TODO(crbug.com/1454853): Consider opt-in by default.
# Assume that users opt in by default; return False only when notify is
# 'NO'.
return WPTDirMetadata(data.get('teamEmail'),
data.get('monorail', {}).get('component'),
data.get('wpt', {}).get('notify') == 'YES')
data.get('wpt', {}).get('notify') != 'NO')
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ def test_read_dir_empty_content(self):

wpt_dir_metadata = extractor.read_dir_metadata(MOCK_WEB_TESTS + 'a/b')
self.assertIsNone(wpt_dir_metadata.team_email)
self.assertFalse(wpt_dir_metadata.should_notify)
self.assertTrue(wpt_dir_metadata.should_notify)
self.assertIsNone(wpt_dir_metadata.component)

def test_read_dir_all_fields(self):
Expand All @@ -266,6 +266,18 @@ def test_read_dir_empty_wpt(self):
self.host.executive = MockExecutive(output=data)
extractor = DirectoryOwnersExtractor(self.host)

wpt_dir_metadata = extractor.read_dir_metadata(MOCK_WEB_TESTS + 'a/b')
self.assertEqual(wpt_dir_metadata.team_email, 'bar')
self.assertTrue(wpt_dir_metadata.should_notify)
self.assertEqual(wpt_dir_metadata.component, 'foo')

def test_read_dir_disable_wpt(self):
data = (
'{"dirs":{"third_party/blink/web_tests/a/b":{"monorail":'
'{"component":"foo"},"teamEmail":"bar","wpt":{"notify":"NO"}}}}')
self.host.executive = MockExecutive(output=data)
extractor = DirectoryOwnersExtractor(self.host)

wpt_dir_metadata = extractor.read_dir_metadata(MOCK_WEB_TESTS + 'a/b')
self.assertEqual(wpt_dir_metadata.team_email, 'bar')
self.assertFalse(wpt_dir_metadata.should_notify)
Expand Down
9 changes: 8 additions & 1 deletion third_party/blink/tools/blinkpy/w3c/import_notifier.py
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,15 @@ def create_bugs_from_new_failures(self, wpt_revision_start,

links_list = '\n[0]: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/testing/web_test_expectations.md\n'

dir_metadata_path = self.host.filesystem.join(
directory, "DIR_METADATA")
epilogue = (
'\nTo opt out of WPT import notifications for this component, '
'add "wpt { notify: NO }" to "%s".' % dir_metadata_path)

description = (prologue + failure_list + expectations_statement +
range_statement + commit_list + links_list)
range_statement + commit_list + links_list +
epilogue)

bug = MonorailIssue.new_chromium_issue(summary,
description,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,10 @@ def mock_run_command(args):
)
self.assertIn('crbug.com/12345 external/wpt/foo/baz.html [ Fail ]',
bugs[0].body['description'].splitlines())
self.assertIn(
'To opt out of WPT import notifications for this component, '
'add "wpt { notify: NO }" to "external/wpt/foo/DIR_METADATA".',
bugs[0].body['description'].splitlines())

def test_file_bug_without_owners(self):
"""A bug should be filed, even without OWNERS next to DIR_METADATA."""
Expand Down

0 comments on commit 183d2ac

Please sign in to comment.