Implement functionality to cc relevant owners.

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

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """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
 
 # Import destination directories (under LayoutTests/imported/).
 WPT_DEST_NAME = 'wpt'
 CSS_DEST_NAME = 'csswg-test'
 
 POLL_DELAY_SECONDS = 300
 
 
 class DepsUpdater(object):
 
     def __init__(self, host):
         self.host = host
         self.executive = host.executive
         self.fs = host.filesystem
         self.finder = WebKitFinder(self.fs)
         self.verbose = False
         self.allow_local_commits = False
         self.keep_w3c_repos_around = False
         self.target = None
         self.auto_update = False
+        self.directory_dict = {}
+        self.email_list = []

[dcampb, 2016/07/27 20:54:57]

Why not assign these two to variables and return them in the script? 
Is there a benefit from making attributes??

[qyearsley, 2016/07/27 21:38:57]

I agree, if these attributes were removed and the functions that set them
(parse_imported_test_changes and parse_directory_owners) were instead changed to
return the desired value, then that might overall be easier to understand.

In general, the fewer attributes a class has, the easier it is to understand.
Every attribute represents some state which could potentially be modified by any
other method on the class; so, whenever you reason about what the value of an
attribute might be, you might have to consider that maybe it might be modified
somewhere else.

Whereas, if you have functions that just take an input and have an output and
have no side effects, then it may be easier to understand.

 
     def main(self, argv=None):
         self.parse_args(argv)
 
         if not self.checkout_is_okay():
             return 1
 
         self.print_('## Noting the current Chromium commit.')
         _, show_ref_output = self.run(['git', 'show-ref', 'HEAD'])
         chromium_commitish = show_ref_output.split()[0]
@@ skipping 17 matching lines @@
         elif self.target == 'css':
             import_commitish = self.update(
                 CSS_DEST_NAME,
                 'https://chromium.googlesource.com/external/w3c/csswg-test.git')
         else:
             raise AssertionError("Unsupported target %s" % self.target)
 
         self.commit_changes_if_needed(chromium_commitish, import_commitish)
         if self.auto_update:
             try_bots = self.host.builders.all_try_builder_names()
+            with open('directory_owners.json') as data_file:

[qyearsley, 2016/07/27 21:38:57]

Would this work if the current working directory is not .../webkitpy/w3c/?

We probably want to use self.finder (a WebKitFinder instance) to get the path to
the file:

  data_file_path = self.finder.path_from_webkit_base(
      'Tools', 'Scripts', 'webkitpy', 'w3c', 'directory_owners.json')

+                self.parse_directory_owners(data_file)
+            self.print_('## Attempting to CC relevant parties.')

[dcampb, 2016/07/27 20:54:57]

Just a comment change:

'Gathering directory owners email to CC'.. or something similar. 

+            _, out = self.run(['git', 'diff', 'master', '--name-only'])
+            self.parse_imported_test_changes(out, self.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'] + self.email_list)
             self.print_('## Triggering try jobs.')
             for try_bot in try_bots:
                 self.run(['git', 'cl', 'try', '-b', try_bot])
             self.print_('## Waiting for Try Job Results')
             has_failing_results = False
             while True:
                 time.sleep(POLL_DELAY_SECONDS)
                 _, out = self.run(['git', 'cl', 'try-results'])
                 results = self.parse_try_job_results(out)
                 if results.get('Started') or results.get('Scheduled'):
                     continue
                 if results.get('Failures'):
                     has_failing_results = True
                 break
             if has_failing_results:
                 self.print_('## Adding test expectations lines to LayoutTests/TestExpectations')
                 script_path = self.path_from_webkit_base('Tools', 'Scripts', 'update-w3c-test-expectations')
                 self.run([self.host.executable, script_path])
                 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'])

[qyearsley, 2016/07/27 21:38:57]

Somewhat separate from this CL:

If possible, it would be nice to extract the body of this if-clause (everything
under `if self.auto_update`) to a separate method. It might also be nice to
extract other sub-functions.

We should consider doing this refactoring in a separate CL.

         return 0
 
     def parse_args(self, argv):
         parser = argparse.ArgumentParser()
         parser.description = __doc__
         parser.add_argument('-v', '--verbose', action='store_true',
                             help='log what we are doing')
         parser.add_argument('--allow-local-commits', action='store_true',
                             help='allow script to run even if we have local commits')
         parser.add_argument('--keep-w3c-repos-around', action='store_true',
@@ skipping 186 matching lines @@
             line = line.strip()
             if line[-1] == ':':
                 result_type = line[:-1]
                 sets[result_type] = set()
             elif line.split()[0] == 'Total:':
                 break
             else:
                 sets[result_type].add(line.split()[0])
         return sets
 
+    def parse_imported_test_changes(self, results, directory_dict):
+        folders_changed = set()
+        for line in results.splitlines():
+            line = line.strip('third_party/WebKit/LayoutTests/')

[qyearsley, 2016/07/27 21:38:56]

I wonder if there's a way that we can avoid having a literal path here.

Some tools we can use:

  self.fs.relpath(path, start): returns the relative path to |path| from start
  self.finder.layout_tests_dir(): Returns the absolute path (I think) to the
layout test dir
  self.finder.chromium_base(): Returns the absolute path (I think) to the
chromium base dir

So maybe something like:

  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)

But there may be a simpler way.

+            directory_sep = line.rfind('/')
+            line = line[:directory_sep]

[qyearsley, 2016/07/27 21:38:56]

This looks like it may be the same as:

  line = self.fs.dirname(line)

You may want to change this to use a new variable name:

  test_directory = self.fs.dirname(line)

+            if line in directory_dict and line not in folders_changed:
+                self.email_list.append('--cc=' + directory_dict[line])
+                folders_changed.add(line)

[dcampb, 2016/07/27 20:54:57]

So I'm guessing this function:

1. Looks for the last '/' in the directory name and takes from 'imported..../'
as the directory
2. Searches for that string in the directory_dict and if the directory hasn't
already been added, it adds it to the email list.

What happens if the directory has no notifications email set to it. Will it try
and CC an empty string?

folders_changed --> directories
line --> directory_path

+
+    def parse_directory_owners(self, data_file):
+        data = json.load(data_file)
+        for dict_set in data:

[dcampb, 2016/07/27 20:54:57]

dict_set --> directory_dictionary

+            self.directory_dict[dict_set['directory']] = dict_set['notification-email']
+
     def check_run(self, command):
         return_code, out = self.run(command)
         if return_code:
             raise Exception('%s failed with exit code %d.' % ' '.join(command), return_code)
         return out