Commit 2c949d86 authored by Kyle Ju's avatar Kyle Ju Committed by Commit Bot

[Import Notifier] Support the new DIR_METADATA format

Modify directory_owners_extractor.py to support the new format. See https://docs.google.com/document/d/12NIyfEpSZvVG95ypw78uA-1EiwSByVVZ-Xyl43EGpLI/preview for more information.

Bug: 1102543
Change-Id: Icaa8f42f056d911f2800186826ccf695bcc988de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2315216
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: default avatarRobert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791328}
parent 6792376b
...@@ -9,11 +9,11 @@ For example, it does not support directives other than email addresses. ...@@ -9,11 +9,11 @@ For example, it does not support directives other than email addresses.
""" """
import collections import collections
import json
import re import re
from blinkpy.common.memoized import memoized from blinkpy.common.memoized import memoized
from blinkpy.common.path_finder import PathFinder from blinkpy.common.path_finder import PathFinder
from blinkpy.common.system.filesystem import FileSystem
# Format of OWNERS files can be found at //src/third_party/depot_tools/owners.py # Format of OWNERS files can be found at //src/third_party/depot_tools/owners.py
# In our use case (under external/wpt), we only process the first enclosing # In our use case (under external/wpt), we only process the first enclosing
...@@ -27,9 +27,10 @@ COMPONENT_REGEXP = r'^# *COMPONENT: *(.+)$' ...@@ -27,9 +27,10 @@ COMPONENT_REGEXP = r'^# *COMPONENT: *(.+)$'
class DirectoryOwnersExtractor(object): class DirectoryOwnersExtractor(object):
def __init__(self, filesystem=None): def __init__(self, host):
self.filesystem = filesystem or FileSystem() self.filesystem = host.filesystem
self.finder = PathFinder(filesystem) self.finder = PathFinder(self.filesystem)
self.executive = host.executive
self.owner_map = None self.owner_map = None
def list_owners(self, changed_files): def list_owners(self, changed_files):
...@@ -128,6 +129,10 @@ class DirectoryOwnersExtractor(object): ...@@ -128,6 +129,10 @@ class DirectoryOwnersExtractor(object):
Returns: Returns:
A string, or None if not found. A string, or None if not found.
""" """
dir_metadata = self._read_dir_metadata(owners_file)
if dir_metadata and dir_metadata.component:
return dir_metadata.component
contents = self._read_text_file(owners_file) contents = self._read_text_file(owners_file)
search = re.search(COMPONENT_REGEXP, contents, re.MULTILINE) search = re.search(COMPONENT_REGEXP, contents, re.MULTILINE)
if search: if search:
...@@ -143,9 +148,99 @@ class DirectoryOwnersExtractor(object): ...@@ -143,9 +148,99 @@ class DirectoryOwnersExtractor(object):
Returns: Returns:
A boolean. A boolean.
""" """
dir_metadata = self._read_dir_metadata(owners_file)
if dir_metadata and dir_metadata.should_notify is not None:
return dir_metadata.should_notify
contents = self._read_text_file(owners_file) contents = self._read_text_file(owners_file)
return bool(re.search(WPT_NOTIFY_REGEXP, contents, re.MULTILINE)) return bool(re.search(WPT_NOTIFY_REGEXP, contents, re.MULTILINE))
@memoized @memoized
def _read_text_file(self, path): def _read_text_file(self, path):
return self.filesystem.read_text_file(path) return self.filesystem.read_text_file(path)
@memoized
def _read_dir_metadata(self, path):
"""Read the content from a path.
Args:
path: An absolute path.
Returns:
A WPTDirMetadata object, or None if not found.
"""
root_path = self.finder.web_tests_dir()
dir_path = self.filesystem.dirname(path)
# dirmd starts with an absolute directory path, `dir_path`, traverses all
# parent directories and stops at `root_path` to find the first available DIR_METADATA
# file. `root_path` is the web_tests directory.
json_data = self.executive.run_command([
self.finder.path_from_depot_tools_base('dirmd'), 'compute',
'-root', root_path, dir_path
])
try:
data = json.loads(json_data)
except ValueError:
return None
relative_path = self.filesystem.relpath(dir_path, root_path)
return WPTDirMetadata(data, relative_path)
class WPTDirMetadata(object):
def __init__(self, data, path):
"""Constructor for WPTDirMetadata.
Args:
data: The output of `dirmd` in _read_dir_metadata; e.g.
{
"dirs":{
"tools/binary_size/libsupersize/testdata/mock_source_directory/base":{
"monorail":{
"project":"chromium",
"component":"Blink>Internal"
},
"teamEmail":"team@chromium.org",
"os":"LINUX",
"wpt":{
"notify":"YES"
}
}
}
}
path: The relative directory path of the DIR_METADATA to the web_tests directory;
see `relative_path` in _read_dir_metadata.
"""
self._data = data
self._path = path
def _get_content(self):
return self._data['dirs'][self._path]
def _is_empty(self):
return len(self._get_content()) == 0
@property
def team_email(self):
if self._is_empty():
return None
# Only returns a single email.
return self._get_content()['teamEmail']
@property
def component(self):
if self._is_empty():
return None
return self._get_content()['monorail']['component']
@property
def should_notify(self):
if self._is_empty():
return None
notify = self._get_content()['wpt']['notify']
# 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'.
return notify == 'YES'
...@@ -3,12 +3,16 @@ ...@@ -3,12 +3,16 @@
# found in the LICENSE file. # found in the LICENSE file.
import unittest import unittest
import json
from blinkpy.common.host_mock import MockHost
from blinkpy.common.path_finder import RELATIVE_WEB_TESTS from blinkpy.common.path_finder import RELATIVE_WEB_TESTS
from blinkpy.common.system.executive_mock import MockExecutive
from blinkpy.common.system.filesystem_mock import MockFileSystem from blinkpy.common.system.filesystem_mock import MockFileSystem
from blinkpy.w3c.directory_owners_extractor import DirectoryOwnersExtractor from blinkpy.w3c.directory_owners_extractor import DirectoryOwnersExtractor, WPTDirMetadata
MOCK_WEB_TESTS = '/mock-checkout/' + RELATIVE_WEB_TESTS MOCK_WEB_TESTS = '/mock-checkout/' + RELATIVE_WEB_TESTS
MOCK_WEB_TESTS_WITHOUT_SLASH = MOCK_WEB_TESTS[:-1]
ABS_WPT_BASE = MOCK_WEB_TESTS + 'external/wpt' ABS_WPT_BASE = MOCK_WEB_TESTS + 'external/wpt'
REL_WPT_BASE = RELATIVE_WEB_TESTS + 'external/wpt' REL_WPT_BASE = RELATIVE_WEB_TESTS + 'external/wpt'
...@@ -16,17 +20,18 @@ REL_WPT_BASE = RELATIVE_WEB_TESTS + 'external/wpt' ...@@ -16,17 +20,18 @@ REL_WPT_BASE = RELATIVE_WEB_TESTS + 'external/wpt'
class DirectoryOwnersExtractorTest(unittest.TestCase): class DirectoryOwnersExtractorTest(unittest.TestCase):
def setUp(self): def setUp(self):
# We always have an OWNERS file at web_tests/external. # We always have an OWNERS file at web_tests/external.
self.filesystem = MockFileSystem(files={ self.host = MockHost()
self.host.filesystem = MockFileSystem(files={
MOCK_WEB_TESTS + 'external/OWNERS': MOCK_WEB_TESTS + 'external/OWNERS':
'ecosystem-infra@chromium.org' 'ecosystem-infra@chromium.org'
}) })
self.extractor = DirectoryOwnersExtractor(self.filesystem) self.extractor = DirectoryOwnersExtractor(self.host)
def _write_files(self, files): def _write_files(self, files):
# Use write_text_file instead of directly assigning to filesystem.files # Use write_text_file instead of directly assigning to filesystem.files
# so that intermediary directories are correctly created, too. # so that intermediary directories are correctly created, too.
for path, contents in files.iteritems(): for path, contents in files.iteritems():
self.filesystem.write_text_file(path, contents) self.host.filesystem.write_text_file(path, contents)
def test_list_owners_combines_same_owners(self): def test_list_owners_combines_same_owners(self):
self._write_files({ self._write_files({
...@@ -156,7 +161,7 @@ class DirectoryOwnersExtractorTest(unittest.TestCase): ...@@ -156,7 +161,7 @@ class DirectoryOwnersExtractorTest(unittest.TestCase):
self.assertIsNone(self.extractor.find_owners_file('third_party')) self.assertIsNone(self.extractor.find_owners_file('third_party'))
def test_extract_owners(self): def test_extract_owners(self):
self.filesystem.files = { self.host.filesystem.files = {
ABS_WPT_BASE + '/foo/OWNERS': ABS_WPT_BASE + '/foo/OWNERS':
'#This is a comment\n' '#This is a comment\n'
'*\n' '*\n'
...@@ -172,7 +177,7 @@ class DirectoryOwnersExtractorTest(unittest.TestCase): ...@@ -172,7 +177,7 @@ class DirectoryOwnersExtractorTest(unittest.TestCase):
['foo@chromium.org', 'bar@chromium.org']) ['foo@chromium.org', 'bar@chromium.org'])
def test_extract_component(self): def test_extract_component(self):
self.filesystem.files = { self.host.filesystem.files = {
ABS_WPT_BASE + '/foo/OWNERS': ABS_WPT_BASE + '/foo/OWNERS':
'# TEAM: some-team@chromium.org\n' '# TEAM: some-team@chromium.org\n'
'# COMPONENT: Blink>Layout\n' '# COMPONENT: Blink>Layout\n'
...@@ -182,7 +187,7 @@ class DirectoryOwnersExtractorTest(unittest.TestCase): ...@@ -182,7 +187,7 @@ class DirectoryOwnersExtractorTest(unittest.TestCase):
'Blink>Layout') 'Blink>Layout')
def test_is_wpt_notify_enabled_true(self): def test_is_wpt_notify_enabled_true(self):
self.filesystem.files = { self.host.filesystem.files = {
ABS_WPT_BASE + '/foo/OWNERS': ABS_WPT_BASE + '/foo/OWNERS':
'# COMPONENT: Blink>Layout\n' '# COMPONENT: Blink>Layout\n'
'# WPT-NOTIFY: true\n' '# WPT-NOTIFY: true\n'
...@@ -191,7 +196,7 @@ class DirectoryOwnersExtractorTest(unittest.TestCase): ...@@ -191,7 +196,7 @@ class DirectoryOwnersExtractorTest(unittest.TestCase):
self.extractor.is_wpt_notify_enabled(ABS_WPT_BASE + '/foo/OWNERS')) self.extractor.is_wpt_notify_enabled(ABS_WPT_BASE + '/foo/OWNERS'))
def test_is_wpt_notify_enabled_false(self): def test_is_wpt_notify_enabled_false(self):
self.filesystem.files = { self.host.filesystem.files = {
ABS_WPT_BASE + '/foo/OWNERS': ABS_WPT_BASE + '/foo/OWNERS':
'# COMPONENT: Blink>Layout\n' '# COMPONENT: Blink>Layout\n'
'# WPT-NOTIFY: false\n' '# WPT-NOTIFY: false\n'
...@@ -200,10 +205,97 @@ class DirectoryOwnersExtractorTest(unittest.TestCase): ...@@ -200,10 +205,97 @@ class DirectoryOwnersExtractorTest(unittest.TestCase):
self.extractor.is_wpt_notify_enabled(ABS_WPT_BASE + '/foo/OWNERS')) self.extractor.is_wpt_notify_enabled(ABS_WPT_BASE + '/foo/OWNERS'))
def test_is_wpt_notify_enabled_absence_is_false(self): def test_is_wpt_notify_enabled_absence_is_false(self):
self.filesystem.files = { self.host.filesystem.files = {
ABS_WPT_BASE + '/foo/OWNERS': ABS_WPT_BASE + '/foo/OWNERS':
'# TEAM: some-team@chromium.org\n' '# TEAM: some-team@chromium.org\n'
'# COMPONENT: Blink>Layout\n' '# COMPONENT: Blink>Layout\n'
} }
self.assertFalse( self.assertFalse(
self.extractor.is_wpt_notify_enabled(ABS_WPT_BASE + '/foo/OWNERS')) self.extractor.is_wpt_notify_enabled(ABS_WPT_BASE + '/foo/OWNERS'))
def test_is_wpt_notify_enabled_with_dir_metadata(self):
self.host.filesystem.files = {
ABS_WPT_BASE + '/foo/OWNERS':
'# TEAM: some-team@chromium.org\n'
'# COMPONENT: Blink>Layout\n'
'# WPT-NOTIFY: true\n'
}
data = (
'{"dirs":{"a/b":{"monorail":'
'{"component":"foo"},"teamEmail":"bar","wpt":{"notify":"YES"}}}}')
self.host.executive = MockExecutive(output=data)
extractor = DirectoryOwnersExtractor(self.host)
self.assertTrue(
extractor.is_wpt_notify_enabled(MOCK_WEB_TESTS + 'a/b/OWNERS'))
def test_is_wpt_notify_enabled_with_dir_metadata_none(self):
self.host.filesystem.files = {
ABS_WPT_BASE + '/foo/OWNERS':
'# COMPONENT: Blink>Layout\n'
'# WPT-NOTIFY: true\n'
}
self.host.executive = MockExecutive(output='error')
extractor = DirectoryOwnersExtractor(self.host)
self.assertTrue(
extractor.is_wpt_notify_enabled(ABS_WPT_BASE + '/foo/OWNERS'))
def test_extract_component_with_dir_metadata(self):
data = (
'{"dirs":{"a/b":{"monorail":'
'{"component":"foo"},"teamEmail":"bar","wpt":{"notify":"YES"}}}}')
self.host.executive = MockExecutive(output=data)
extractor = DirectoryOwnersExtractor(self.host)
self.assertEqual(
extractor.extract_component(MOCK_WEB_TESTS + 'a/b/OWNERS'), 'foo')
def test_read_dir_metadata_success(self):
data = (
'{"dirs":{"a/b":{"monorail":'
'{"component":"foo"},"teamEmail":"bar","wpt":{"notify":"YES"}}}}')
self.host.executive = MockExecutive(output=data)
extractor = DirectoryOwnersExtractor(self.host)
wpt_dir_metadata = extractor._read_dir_metadata(MOCK_WEB_TESTS +
'a/b/OWNERS')
self.assertEqual(self.host.executive.full_calls[0].args, [
'dirmd', 'compute', '-root', MOCK_WEB_TESTS_WITHOUT_SLASH,
MOCK_WEB_TESTS + 'a/b'
])
self.assertEqual(wpt_dir_metadata.team_email, 'bar')
self.assertEqual(wpt_dir_metadata.should_notify, True)
self.assertEqual(wpt_dir_metadata.component, 'foo')
def test_read_dir_metadata_none(self):
self.host.executive = MockExecutive(output='error')
extractor = DirectoryOwnersExtractor(self.host)
wpt_dir_metadata = extractor._read_dir_metadata(MOCK_WEB_TESTS +
'a/b/OWNERS')
self.assertEqual(self.host.executive.full_calls[0].args, [
'dirmd', 'compute', '-root', MOCK_WEB_TESTS_WITHOUT_SLASH,
MOCK_WEB_TESTS + 'a/b'
])
self.assertEqual(wpt_dir_metadata, None)
class WPTDirMetadataTest(unittest.TestCase):
def test_WPTDirMetadata_empty(self):
empty_data = '{"dirs":{"a/b":{}}}'
wpt_dir_metadata = WPTDirMetadata(json.loads(empty_data), 'a/b')
self.assertEqual(wpt_dir_metadata.team_email, None)
self.assertEqual(wpt_dir_metadata.should_notify, None)
self.assertEqual(wpt_dir_metadata.component, None)
def test_WPTDirMetadata_non_empty(self):
data = (
'{"dirs":{"a/b":{"monorail":'
'{"component":"foo"},"teamEmail":"bar","wpt":{"notify":"YES"}}}}')
wpt_dir_metadata = WPTDirMetadata(json.loads(data), 'a/b')
self.assertEqual(wpt_dir_metadata.team_email, 'bar')
self.assertEqual(wpt_dir_metadata.should_notify, True)
self.assertEqual(wpt_dir_metadata.component, 'foo')
...@@ -36,7 +36,7 @@ class ImportNotifier(object): ...@@ -36,7 +36,7 @@ class ImportNotifier(object):
self._monorail_api = MonorailAPI self._monorail_api = MonorailAPI
self.default_port = host.port_factory.get() self.default_port = host.port_factory.get()
self.finder = PathFinder(host.filesystem) self.finder = PathFinder(host.filesystem)
self.owners_extractor = DirectoryOwnersExtractor(host.filesystem) self.owners_extractor = DirectoryOwnersExtractor(host)
self.new_failures_by_directory = defaultdict(list) self.new_failures_by_directory = defaultdict(list)
def main(self, def main(self,
......
...@@ -524,7 +524,7 @@ class TestImporter(object): ...@@ -524,7 +524,7 @@ class TestImporter(object):
"""Returns a mapping of email addresses to owners of changed tests.""" """Returns a mapping of email addresses to owners of changed tests."""
_log.info('Gathering directory owners emails to CC.') _log.info('Gathering directory owners emails to CC.')
changed_files = self.chromium_git.changed_files() changed_files = self.chromium_git.changed_files()
extractor = DirectoryOwnersExtractor(self.fs) extractor = DirectoryOwnersExtractor(self.host)
return extractor.list_owners(changed_files) return extractor.list_owners(changed_files)
def _cl_description(self, directory_owners): def _cl_description(self, directory_owners):
......
monorail {
project: "chromium"
component: "Blink>Infra>Ecosystem"
}
team_email: "ecosystem-infra@chromium.org"
os: LINUX
wpt {
notify: YES
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment