Implement functionality to cc relevant owners.

third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py

Index: third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
index 5e43fd1fadd0a88a907116f68af3efdc160fc045..e91a7c100926c54971d43a273193ff8d70545fc1 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
@@ -5,6 +5,7 @@
 """Pull latest revisions of a W3C test repo and make a local commit."""
 
 import argparse
+import json
 import time
 
 from webkitpy.common.webkit_finder import WebKitFinder
@@ -65,8 +66,14 @@ class DepsUpdater(object):
         self.commit_changes_if_needed(chromium_commitish, import_commitish)
         if self.auto_update:
             try_bots = self.host.builders.all_try_builder_names()
+            data_file_path = self.finder.path_from_webkit_base('Tools', 'Scripts', 'webkitpy', 'w3c', 'directory_owners.json')
+            with open(data_file_path) as data_file:
+                directory_dict = self.parse_directory_owners(data_file)

[qyearsley, 2016/07/28 22:31:18]

To make `parse_directory_owners` more single-purpose (and probably easier to
test), maybe it could take the list of dicts parsed from the file, rather than
the file object -- so this here would be:

     with open(data_file_path) as data_file:
        directory_dict = self.parse_directory_owners(json.load(data_file))

+            self.print_('Gathering directory owners email to CC')
+            _, out = self.run(['git', 'diff', 'master', '--name-only'])
+            email_list = self.parse_imported_test_changes(out, directory_dict)
             self.print_('## Uploading change list.')
-            self.check_run(['git', 'cl', 'upload', '-f', '-m', 'W3C auto test importer'])
+            self.check_run(['git', 'cl', 'upload', '-f', '-m', 'W3C auto test importer'] + email_list)
             self.print_('## Triggering try jobs.')
             for try_bot in try_bots:
                 self.run(['git', 'cl', 'try', '-b', try_bot])
@@ -88,7 +95,7 @@ class DepsUpdater(object):
                 self.check_run(['git', 'commit', '-a', '-m', '\'Modified Test Expectations from W3C Test Auto-roller\''])
                 self.check_run(['git', 'cl', 'upload', '-m', '\'Wrote lines to TestExpectations\''])
             else:
-                self.print_('No Failures, committing patch.')
+                self.print_('No Failures, landing patch.')
             quit()
             self.run(['git', 'cl', 'land', '-f'])
         return 0
@@ -297,6 +304,29 @@ class DepsUpdater(object):
                 sets[result_type].add(line.split()[0])
         return sets
 
+    def parse_imported_test_changes(self, results, directory_dict):

[qyearsley, 2016/07/28 22:31:18]

Some things which may make this function easier to read:
 - A docstring would be helpful
 - The argument `results` isn't very clear
 - The word "parse" in the method name makes it sound like you're return some
structured data based on some string. Which is somewhat true, but I'd say the
main thing that this function does is get email addresses to CC based on owners
of files.

Would it make sense to change this to something like
`get_emails_to_cc(changed_files, directory_to_email_map)` where changed_files is
a list of changed files?

+        email_list = []
+        directories = set()
+        for line in results.splitlines():
+            layout_tests_relative_path = self.fs.relpath(self.finder.layout_tests_dir(), self.finder.chromium_base())
+            test_path = self.fs.relpath(line, layout_tests_relative_path)
+            print test_path
+            test_path = self.fs.dirname(test_path)
+            print test_path

[qyearsley, 2016/07/28 22:31:18]

Remember to remove debugging print lines :-)

+            test_path = test_path.strip('../')
+            if test_path in directory_dict and test_path not in directories:
+                email_list.append('--cc=' + directory_dict[test_path])

[qyearsley, 2016/07/28 22:31:18]

The name email_list sounds like it's just a list of emails, but it's actually a
list of extra args to `git cl upload` to add email addresses to CC.

It may be nice to change this to just return a list of email addresses, since
it's easy to add "--cc=" to each of them later; for example:

  self.check_run(['git', 'cl', 'upload', ...] +
                 ['--cc=' + email for email in email_list])

  

+                directories.add(test_path)
+        return email_list
+
+    def parse_directory_owners(self, data_file):
+        directory_dict = {}
+        data = json.load(data_file)
+        for dict_set in data:
+            if dict_set['notification-email']:
+                directory_dict[dict_set['directory']] = dict_set['notification-email']
+        return directory_dict
+
     def check_run(self, command):
         return_code, out = self.run(command)
         if return_code: