Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
339623002:
Added support for versioning of layout test results of run-webkit-tests runs (Closed)
DescriptionAdded support for versioning of layout test results of run-webkit-tests runs.
And to generate a dashboard of all archived layout test results.
On local runs of run-webkit-tests, the results are overwritten. If required, we cannot refer to the older
generated which might be required many times.
Instead we can archive the results by adding a time-stamp
to the folder name on each run which will help us track the change in results with each iteration of results run.
And also we can generate a dashboard out of all the archived results to validate each individual test over many iterations of testing.
BUG=305508
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179332
Patch Set 1 #Patch Set 2 : #
Total comments: 18
Patch Set 3 : Addressing comments #
Total comments: 40
Patch Set 4 : Addressing comments #
Total comments: 4
Patch Set 5 : #
Total comments: 2
Patch Set 6 : Added unittest #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #
Total comments: 8
Patch Set 9 : Addressing comments #Patch Set 10 : Added layout test #Messages
Total messages: 41 (1 generated)
Kindly review Some clean up is still required especially in dashboard.html I wanted to get feedback on the overall approach for this feature.
Dirk or Ojan are the correct reviewers.
As I think this mostly has to do w/ storing and reflecting the results in the dashboard object and the html and json, I'm going to punt most of this to Ojan. I do have a few concerns about the python implementation that I've noted below. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:44: from webkitpy.common.checkout.scm.detection import SCMDetector I think this import is unneeded. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:46: from webkitpy.common.system.filesystem import FileSystem this should be unneeded as well. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:84: self._scm = None These scm fields don't seem to be used? https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:167: return archived_path I don't know that we should assume we can write outside of self._results_directory. We should consider other options here. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:283: summarized_full_results = test_run_results.summarize_results(self._port, self._expectations, initial_results, retry_results, enabled_pixel_tests_in_retry, False, self.ARCHIVED_PATH) see comment in test_run_results. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right): https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:33: from webkitpy.common.system.filesystem import FileSystem there's no need to import FileSystem directly. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:39: class GenerateDashBoard: nit: this should inherit from (object). https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py (right): https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py:166: def summarize_results(port_obj, expectations, initial_results, retry_results, enabled_pixel_tests_in_retry, only_include_failing=False, archived_results_path=None, revision_info=''): it feels a bit odd to me that the results object would know about the previous results. What are the advantages of this approach vs keeping things completely separate? https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:176: default=True, help="Archive the test results for later access."), did you mean for the default to be False? Otherwise this switch has no effect ...
I'm busy with the code yellow and will be on vacation for a couple weeks. So, I won't be able to look at this for at least another month. Sorry.
On 2014/06/17 23:33:09, ojan-only-code-yellow-reviews wrote: > I'm busy with the code yellow and will be on vacation for a couple weeks. So, I > won't be able to look at this for at least another month. Sorry. Friendly Ping!!
Kindly Review !!! Thanks. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:44: from webkitpy.common.checkout.scm.detection import SCMDetector On 2014/06/17 17:58:58, Dirk Pranke wrote: > I think this import is unneeded. Done. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:46: from webkitpy.common.system.filesystem import FileSystem On 2014/06/17 17:58:58, Dirk Pranke wrote: > this should be unneeded as well. Done. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:84: self._scm = None On 2014/06/17 17:58:58, Dirk Pranke wrote: > These scm fields don't seem to be used? Done. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:167: return archived_path On 2014/06/17 17:58:58, Dirk Pranke wrote: > I don't know that we should assume we can write outside of > self._results_directory. We should consider other options here. If the archived results are enabled by default then we can create a sub directory in self._results_directory for each archived results. The current directory structure is maintained as it is still an experimental feature. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:283: summarized_full_results = test_run_results.summarize_results(self._port, self._expectations, initial_results, retry_results, enabled_pixel_tests_in_retry, False, self.ARCHIVED_PATH) On 2014/06/17 17:58:57, Dirk Pranke wrote: > see comment in test_run_results. Done. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right): https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:33: from webkitpy.common.system.filesystem import FileSystem On 2014/06/17 17:58:58, Dirk Pranke wrote: > there's no need to import FileSystem directly. Done. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:39: class GenerateDashBoard: On 2014/06/17 17:58:58, Dirk Pranke wrote: > nit: this should inherit from (object). Done. https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py (right): https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py:166: def summarize_results(port_obj, expectations, initial_results, retry_results, enabled_pixel_tests_in_retry, only_include_failing=False, archived_results_path=None, revision_info=''): On 2014/06/17 17:58:58, Dirk Pranke wrote: > it feels a bit odd to me that the results object would know about the previous > results. What are the advantages of this approach vs keeping things completely > separate? Yes, things can be seperate there need not be any link .The script does the ordering of the results based on the directory names (which have timestamps). https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/339623002/diff/40001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:176: default=True, help="Archive the test results for later access."), On 2014/06/17 17:58:58, Dirk Pranke wrote: > did you mean for the default to be False? Otherwise this switch has no effect > ... Yes it must be default=False
lots more comments , now that I'm taking a closer look at this and have played with it a bit. Once we get this into good enough shape we can either ping ojan to take a look, or just land it and make him look at it later if he cares. https://codereview.chromium.org/339623002/diff/60001/LayoutTests/fast/harness... File LayoutTests/fast/harness/dashboard.html (right): https://codereview.chromium.org/339623002/diff/60001/LayoutTests/fast/harness... LayoutTests/fast/harness/dashboard.html:1: <!DOCTYPE html> There's a fair amount of duplication between dashboard.html and results.html; can we figure out how to share a lot of it in .js or .css files? See also my comments elsewhere about potentially moving dashboard.html *inside* layout-test-results. https://codereview.chromium.org/339623002/diff/60001/LayoutTests/fast/harness... LayoutTests/fast/harness/dashboard.html:299: var prefix = hasTrac ? 'trac.webkit.org/.../' : ''; links to trac.webkit.org can't be right ... is this just stale copy/paste code? https://codereview.chromium.org/339623002/diff/60001/LayoutTests/fast/harness... LayoutTests/fast/harness/dashboard.html:329: function shouldUseTracLinks() same comment https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:81: self.ARCHIVED_PATH = None ARCHIVED_PATH isn't a constant; don't use upper case for it. It also looks like it isn't actually used at all ... REVISION_INFO doesn't look like it is used at all. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:171: _log.info("Found no result.html in the folder, ignoring this result and overwritten the folder...") Don't use both a print and a _log.info(). Just use log.warning() (which is the appropriate thing for this). I would also rewrite this as 'No results.html file found in previous run, skipping it.'. Put the try/except around just line 159, to be clear about what you're actually checking. Right now you might get an OSError from the filesystem.move() call, too, and you'll print the wrong warning for it. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:199: # FIXME:: Here the condition has to be (archive_prev_results) and (total_archived_results < MAX_ARCHIVE_RESULTS) This comment is referring to things that don't exist. However, it seems to be getting at the idea that we don't want to archive an indefinite number of results, which seems like a good idea? Maybe rewrite the FIXME to be clearer, though I would just implement this logic as part of this patch. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:202: self.ARCHIVED_PATH = self._rename_results_folder_if_required() the actual method is renaming the folder unconditionally, so I would just call it 'self._rename_old_results_folder' (no "_if_required"). Also, if someone passes --clobber-old-results and --enable-versioned-results, you'll clobber the data and then rename it; probably not what we want. I would probably either make the two flags mutually exclusive (and error out when you parse the arguments), or make enable-versioned-results override --clobber and log a warning to say that that's what you're doing (I think I like the latter option better). https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right): https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:33: from webkitpy.layout_tests.port import configuration_options, platform_options Do you need this import? I don't think so; you probably don't need the logging and string imports, either. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:40: "A class for generating the Dashboard from the list of archived results" This docstring doesn't tell us much; I'd probably delete it. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:46: self._release_directory = self._filesystem.join(self._filesystem.dirname(self._results_directory), '') You don't need the .join() here; dirname() gives you what you need. 'release_directory' is misleading if you are writing into a custom results directory or using a debug build. You need to decide if you need to support versioned results with custom directories, and, if so, how that should work. One way that might be good enough would be to move dashboard.html and archived_results.json into the layout-test-results directory, and then link back to prior layout-test-results directories if you find them next to the current directory. This would remove the need to overwrite archived_results.json on each run, and would keep you from writing to files outside of layout-test-results (though you would still be renaming directories). It would also be good to have a link from 'results.html' to the dashboard directly, and if you keep results.html and dashboard.html next to each other, you can probably reuse some of the javascript between the two files. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:47: self._input_json_data = "" Is this supposed to be a string or a dict? Why not just use self._result_directory_list instead (here and below)? https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:52: def _add_individual_result_links(self, file_list): this is actually a list of directories, not a list of files, right? You should probably fix the parameter name to be clearer, if so (e.g., 'results_directories'). https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:59: if ~(self._filesystem.exists(dashboard_file)): this should be 'if not self._filesystem.exists(dashboard_file):' . Don't use the outside parens, and definitely don't use '~' :). https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:65: json_file_list = [] json_file_list isn't a list of json files, it's a list of results directories. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:70: #Read the current Layout Test Results if you are going to add comments, add a blank line before them, and put a space between the '#' and your text (it's good style to break up a long-ish method into blocks of lines like paragraphs). In this case, I don't think the comments are adding much beyond the fairly-clear code and method names, so I'd omit them completely. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:73: input_json_string = input_json_string[12:-2] # Remove preceeding string ADD_RESULTS( and ); at the end I would extract lines 71-73 into a routine and reuse it on lines 80-82 as well. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:97: exit() printing an error and calling exit is actually less useful than just letting the exception be raised. I would delete this whole routine (see following comment). https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:100: def _process_json_result(self, json_object): what are you 'processing' here? can you use a more specific verb? It looks like you're actually trying to figure out if a given test passed or not? is the json_object actually a result object? can you give that a more specific name as well? https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:101: actual = self._get_value(json_object, "actual") just use actual = json_object['actual'] here and as follows. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:110: pass You can replace lines 107-110 with hasStderr = json_object.get('has_stderr', 'false') You can actually replace all of lines 106-113 with: return 'HASSTDERR' if json_object.get('has_stderr') == 'true' else 'PASS' https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:149: self._add_archived_results(json_object, row) just replace line 144 with lines 147-149 and replace the 'break' with a 'return', and you don't need 'flag' (and things are clearer).
Kindly Review, I will modify dashboard.html and results.html and upload it soon in the next patch Thanks, Shyam https://codereview.chromium.org/339623002/diff/60001/LayoutTests/fast/harness... File LayoutTests/fast/harness/dashboard.html (right): https://codereview.chromium.org/339623002/diff/60001/LayoutTests/fast/harness... LayoutTests/fast/harness/dashboard.html:1: <!DOCTYPE html> On 2014/07/15 20:50:00, Dirk Pranke wrote: > There's a fair amount of duplication between dashboard.html and results.html; > can we figure out how to share a lot of it in .js or .css files? See also my > comments elsewhere about potentially moving dashboard.html *inside* > layout-test-results. Yes both dashboard.html and results.html can share code in .js and .css. I will modify this in next patch https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:81: self.ARCHIVED_PATH = None On 2014/07/15 20:50:00, Dirk Pranke wrote: > ARCHIVED_PATH isn't a constant; don't use upper case for it. It also looks like > it isn't actually used at all ... > > REVISION_INFO doesn't look like it is used at all. Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:171: _log.info("Found no result.html in the folder, ignoring this result and overwritten the folder...") On 2014/07/15 20:50:00, Dirk Pranke wrote: > Don't use both a print and a _log.info(). Just use log.warning() (which is the > appropriate thing for this). > > I would also rewrite this as 'No results.html file found in previous run, > skipping it.'. > > Put the try/except around just line 159, to be clear about what you're actually > checking. Right now you might get an OSError from the filesystem.move() call, > too, and you'll print the wrong warning for it. Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:199: # FIXME:: Here the condition has to be (archive_prev_results) and (total_archived_results < MAX_ARCHIVE_RESULTS) On 2014/07/15 20:50:00, Dirk Pranke wrote: > This comment is referring to things that don't exist. However, it seems to be > getting at the idea that we don't want to archive an indefinite number of > results, which seems like a good idea? Maybe rewrite the FIXME to be clearer, > though I would just implement this logic as part of this patch. Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:202: self.ARCHIVED_PATH = self._rename_results_folder_if_required() On 2014/07/15 20:50:00, Dirk Pranke wrote: > the actual method is renaming the folder unconditionally, so I would just call > it 'self._rename_old_results_folder' (no "_if_required"). > > Also, if someone passes --clobber-old-results and --enable-versioned-results, > you'll clobber the data and then rename it; probably not what we want. > > I would probably either make the two flags mutually exclusive (and error out > when you parse the arguments), or make enable-versioned-results override > --clobber and log a warning to say that that's what you're doing (I think I like > the latter option better). Making the flags --enable-versioned-results --clobber-old-results mutually exclusive https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right): https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:33: from webkitpy.layout_tests.port import configuration_options, platform_options On 2014/07/15 20:50:01, Dirk Pranke wrote: > Do you need this import? I don't think so; you probably don't need the logging > and string imports, either. Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:40: "A class for generating the Dashboard from the list of archived results" On 2014/07/15 20:50:01, Dirk Pranke wrote: > This docstring doesn't tell us much; I'd probably delete it. Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:46: self._release_directory = self._filesystem.join(self._filesystem.dirname(self._results_directory), '') On 2014/07/15 20:50:00, Dirk Pranke wrote: > You don't need the .join() here; dirname() gives you what you need. > > 'release_directory' is misleading if you are writing into a custom results > directory or using a debug build. > > You need to decide if you need to support versioned results with custom > directories, and, if so, how that should work. One way that might be good enough > would be to move dashboard.html and archived_results.json into the > layout-test-results directory, and then link back to prior layout-test-results > directories if you find them next to the current directory. > > This would remove the need to overwrite archived_results.json on each run, and > would keep you from writing to files outside of layout-test-results (though you > would still be renaming directories). > > It would also be good to have a link from 'results.html' to the dashboard > directly, and if you keep results.html and dashboard.html next to each other, > you can probably reuse some of the javascript between the two files. Renaming it to self._results_directory_path Yes if we move the dashboard and archived results json inside the results directory we need not write outside the layout-results-directory. This can allow to have unique dashboard for each test. I will modify this in next patch. I already added a link from results.html to dashboard. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:47: self._input_json_data = "" On 2014/07/15 20:50:01, Dirk Pranke wrote: > Is this supposed to be a string or a dict? Why not just use > self._result_directory_list instead (here and below)? It is a dict. Renaming it ro self._current_result_json_dict https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:52: def _add_individual_result_links(self, file_list): On 2014/07/15 20:50:00, Dirk Pranke wrote: > this is actually a list of directories, not a list of files, right? You should > probably fix the parameter name to be clearer, if so (e.g., > 'results_directories'). Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:59: if ~(self._filesystem.exists(dashboard_file)): On 2014/07/15 20:50:01, Dirk Pranke wrote: > this should be 'if not self._filesystem.exists(dashboard_file):' . Don't use the > outside parens, and definitely don't use '~' :). Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:65: json_file_list = [] On 2014/07/15 20:50:01, Dirk Pranke wrote: > json_file_list isn't a list of json files, it's a list of results directories. Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:70: #Read the current Layout Test Results On 2014/07/15 20:50:00, Dirk Pranke wrote: > if you are going to add comments, add a blank line before them, and put a space > between the '#' and your text (it's good style to break up a long-ish method > into blocks of lines like paragraphs). In this case, I don't think the comments > are adding much beyond the fairly-clear code and method names, so I'd omit them > completely. Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:97: exit() On 2014/07/15 20:50:01, Dirk Pranke wrote: > printing an error and calling exit is actually less useful than just letting the > exception be raised. I would delete this whole routine (see following comment). Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:100: def _process_json_result(self, json_object): On 2014/07/15 20:50:01, Dirk Pranke wrote: > what are you 'processing' here? can you use a more specific verb? It looks like > you're actually trying to figure out if a given test passed or not? is the > json_object actually a result object? can you give that a more specific name as > well? Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:101: actual = self._get_value(json_object, "actual") On 2014/07/15 20:50:01, Dirk Pranke wrote: > just use actual = json_object['actual'] here and as follows. Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:110: pass On 2014/07/15 20:50:00, Dirk Pranke wrote: > You can replace lines 107-110 with hasStderr = json_object.get('has_stderr', > 'false') > > You can actually replace all of lines 106-113 with: > > return 'HASSTDERR' if json_object.get('has_stderr') == 'true' else 'PASS' Done. https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:149: self._add_archived_results(json_object, row) On 2014/07/15 20:50:01, Dirk Pranke wrote: > just replace line 144 with lines 147-149 and replace the 'break' with a > 'return', and you don't need 'flag' (and things are clearer). Done.
getting closer ... https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right): https://codereview.chromium.org/339623002/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:47: self._input_json_data = "" On 2014/07/16 14:37:37, shyamp wrote: > On 2014/07/15 20:50:01, Dirk Pranke wrote: > > Is this supposed to be a string or a dict? Why not just use > > self._result_directory_list instead (here and below)? > > It is a dict. > Renaming it ro self._current_result_json_dict If it's a dict, why are you assigning a string to it? https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:168: return archived_path Do you need to return the archived_path? You're not using the return value ... https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right): https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:46: self._current_result_json_dict['result_links'] = archived_results_file_list So, if I'm following this, _current_result_json_dict() is basically failing_results.json + result_links, right? And result_links is only used when you write the value back out in generate()? This approach seems a little hard to follow; can you store result_links in a separate variable, and then add it to current_result_json_dict() only in generate()? I think that might be a bit clearer ... Also, if you do end up combining dashboard.html and results.html to share more code, it's probably better (if possible) to leave failing_results.html alone; so, I'd consider just writing the list of results out to a separate file and loading that instead. Make sense?
PTAL Thanks Shyam https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:168: return archived_path On 2014/07/16 19:32:44, Dirk Pranke wrote: > Do you need to return the archived_path? You're not using the return value ... Done. https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right): https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:46: self._current_result_json_dict['result_links'] = archived_results_file_list On 2014/07/16 19:32:44, Dirk Pranke wrote: > So, if I'm following this, _current_result_json_dict() is basically > failing_results.json + result_links, right? > > And result_links is only used when you write the value back out in generate()? > > This approach seems a little hard to follow; can you store result_links in a > separate variable, and then add it to current_result_json_dict() only in > generate()? I think that might be a bit clearer ... > > Also, if you do end up combining dashboard.html and results.html to share more > code, it's probably better (if possible) to leave failing_results.html alone; > so, I'd consider just writing the list of results out to a separate file and > loading that instead. Make sense? _current_result_json_dict() = failing_results.json + result_links + old results For example consider one test result in failing_list.json "2d.canvas.readonly.html": { "actual": "PASS", "expected": "PASS", "has_stderr": true, "time": 4.0 } in archived_results.json the content will be the above data plus pass/fail status in previous iterations "2d.canvas.readonly.html": { "actual": "PASS", "archived_results": [ "PASS", "PASS", "PASS", "PASS" ], "expected": "PASS", "has_stderr": true, "time": 4.0 } Also we can combine failing_results.json and archived_results.json into one file and results.html and dashboard.html will extract the required data separately. results_links are used by the js in dashboard.html to provide links to old results.
On 2014/07/18 06:53:25, shyamp wrote: > https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... > File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right): > > https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... > Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:46: > self._current_result_json_dict['result_links'] = archived_results_file_list > On 2014/07/16 19:32:44, Dirk Pranke wrote: > > So, if I'm following this, _current_result_json_dict() is basically > > failing_results.json + result_links, right? > > > > And result_links is only used when you write the value back out in generate()? > > > > This approach seems a little hard to follow; can you store result_links in a > > separate variable, and then add it to current_result_json_dict() only in > > generate()? I think that might be a bit clearer ... > > > > Also, if you do end up combining dashboard.html and results.html to share more > > code, it's probably better (if possible) to leave failing_results.html alone; > > so, I'd consider just writing the list of results out to a separate file and > > loading that instead. Make sense? > > _current_result_json_dict() = failing_results.json + result_links + old results > > For example > consider one test result in failing_list.json > "2d.canvas.readonly.html": { > "actual": "PASS", > "expected": "PASS", > "has_stderr": true, > "time": 4.0 > } > in archived_results.json the content will be the above data plus pass/fail > status in previous iterations > "2d.canvas.readonly.html": { > "actual": "PASS", > "archived_results": [ > "PASS", > "PASS", > "PASS", > "PASS" > ], > "expected": "PASS", > "has_stderr": true, > "time": 4.0 > } > I see; I didn't realize that you were merging things all the way down the trie. This seems like an iffy design to me. The dashboard doesn't use the other data in failing_results, and results.html doesn't use the archived fields, so I'd probably be inclined to keep the two data sets separate. Also, failing_results.json gets uploaded to the server, so I don't want to upload a bunch of data the server doesn't need and might get confused by. Is there a reason you need to combine the data in-memory that I'm missing?
On 2014/07/18 17:08:57, Dirk Pranke wrote: > On 2014/07/18 06:53:25, shyamp wrote: > > > https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... > > File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py > (right): > > > > > https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... > > Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:46: > > self._current_result_json_dict['result_links'] = archived_results_file_list > > On 2014/07/16 19:32:44, Dirk Pranke wrote: > > > So, if I'm following this, _current_result_json_dict() is basically > > > failing_results.json + result_links, right? > > > > > > And result_links is only used when you write the value back out in > generate()? > > > > > > This approach seems a little hard to follow; can you store result_links in a > > > separate variable, and then add it to current_result_json_dict() only in > > > generate()? I think that might be a bit clearer ... > > > > > > Also, if you do end up combining dashboard.html and results.html to share > more > > > code, it's probably better (if possible) to leave failing_results.html > alone; > > > so, I'd consider just writing the list of results out to a separate file and > > > loading that instead. Make sense? > > > > _current_result_json_dict() = failing_results.json + result_links + old > results > > > > For example > > consider one test result in failing_list.json > > "2d.canvas.readonly.html": { > > "actual": "PASS", > > "expected": "PASS", > > "has_stderr": true, > > "time": 4.0 > > } > > in archived_results.json the content will be the above data plus pass/fail > > status in previous iterations > > "2d.canvas.readonly.html": { > > "actual": "PASS", > > "archived_results": [ > > "PASS", > > "PASS", > > "PASS", > > "PASS" > > ], > > "expected": "PASS", > > "has_stderr": true, > > "time": 4.0 > > } > > > > I see; I didn't realize that you were merging things all the way down the trie. > > This seems like an iffy design to me. The dashboard doesn't use the other data > in failing_results, and results.html doesn't use the archived fields, so I'd > probably be inclined to keep the two data sets separate. > > Also, failing_results.json gets uploaded to the server, so I don't want to > upload a bunch of data the server doesn't need and might get confused by. > > Is there a reason you need to combine the data in-memory that I'm missing? There is no need to combine both the results. It would be better to keep them separate. So there will be a dashboard.html and a separate archived_results.json inside the layout_test_results folder. Initially i thought i would merge failing_results.json and archived_results.json into one file like I mentioned previously but like you said it is better to keep them separate.
On 2014/07/18 17:55:33, shyamp wrote: > On 2014/07/18 17:08:57, Dirk Pranke wrote: > > On 2014/07/18 06:53:25, shyamp wrote: > > > > > > https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... > > > File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py > > (right): > > > > > > > > > https://codereview.chromium.org/339623002/diff/80001/Tools/Scripts/webkitpy/l... > > > Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:46: > > > self._current_result_json_dict['result_links'] = archived_results_file_list > > > On 2014/07/16 19:32:44, Dirk Pranke wrote: > > > > So, if I'm following this, _current_result_json_dict() is basically > > > > failing_results.json + result_links, right? > > > > > > > > And result_links is only used when you write the value back out in > > generate()? > > > > > > > > This approach seems a little hard to follow; can you store result_links in > a > > > > separate variable, and then add it to current_result_json_dict() only in > > > > generate()? I think that might be a bit clearer ... > > > > > > > > Also, if you do end up combining dashboard.html and results.html to share > > more > > > > code, it's probably better (if possible) to leave failing_results.html > > alone; > > > > so, I'd consider just writing the list of results out to a separate file > and > > > > loading that instead. Make sense? > > > > > > _current_result_json_dict() = failing_results.json + result_links + old > > results > > > > > > For example > > > consider one test result in failing_list.json > > > "2d.canvas.readonly.html": { > > > "actual": "PASS", > > > "expected": "PASS", > > > "has_stderr": true, > > > "time": 4.0 > > > } > > > in archived_results.json the content will be the above data plus pass/fail > > > status in previous iterations > > > "2d.canvas.readonly.html": { > > > "actual": "PASS", > > > "archived_results": [ > > > "PASS", > > > "PASS", > > > "PASS", > > > "PASS" > > > ], > > > "expected": "PASS", > > > "has_stderr": true, > > > "time": 4.0 > > > } > > > > > > > I see; I didn't realize that you were merging things all the way down the > trie. > > > > This seems like an iffy design to me. The dashboard doesn't use the other data > > in failing_results, and results.html doesn't use the archived fields, so I'd > > probably be inclined to keep the two data sets separate. > > > > Also, failing_results.json gets uploaded to the server, so I don't want to > > upload a bunch of data the server doesn't need and might get confused by. > > > > Is there a reason you need to combine the data in-memory that I'm missing? > > There is no need to combine both the results. > It would be better to keep them separate. So there will be a dashboard.html and > a separate archived_results.json inside the layout_test_results folder. > > Initially i thought i would merge failing_results.json and archived_results.json > into one file like I mentioned previously but like you said it is better to keep > them separate. PTAL Moved the dashboard.html and archived_results.json inside the results folder. Now archived_results.json = processed_archived_results + results_links. failing_results.json is left untouched as per previous discussions. Modified dashboard.html will update it more in next patch. Thanks, Shyam Patro
looks fine so far ... https://codereview.chromium.org/339623002/diff/100001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right): https://codereview.chromium.org/339623002/diff/100001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:65: self._current_result_json_dict['tests'] = json.loads(input_json_string)["tests"] nit: single quotes in both cases
PTAL. Thanks, Shyam Patro https://codereview.chromium.org/339623002/diff/100001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right): https://codereview.chromium.org/339623002/diff/100001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:65: self._current_result_json_dict['tests'] = json.loads(input_json_string)["tests"] On 2014/07/22 20:02:05, Dirk Pranke wrote: > nit: single quotes in both cases Done.
https://codereview.chromium.org/339623002/diff/120001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/process_json_data.py (right): https://codereview.chromium.org/339623002/diff/120001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/process_json_data.py:1: # Copyright (C) 2014 Google Inc. All rights reserved. I would leave this as part of generate_results_dashboard.py . No other module will be calling this one, and it is perfectly fine and encouraged to group multiple classes into a single file when they belong together. Let me know if I'm missing some advantage to this being a separate file.
PTAL, Thanks Shyam Patro https://codereview.chromium.org/339623002/diff/120001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/process_json_data.py (right): https://codereview.chromium.org/339623002/diff/120001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/process_json_data.py:1: # Copyright (C) 2014 Google Inc. All rights reserved. On 2014/07/25 00:42:55, Dirk Pranke wrote: > I would leave this as part of generate_results_dashboard.py . No other module > will be calling this one, and it is perfectly fine and encouraged to group > multiple classes into a single file when they belong together. > > Let me know if I'm missing some advantage to this being a separate file. Done.
On 2014/07/25 05:14:35, shyamp wrote: > Done. Seems fine. I've lost track, though: what's left for you to do before we think this is ready and all the feedback has been incorporated?
On 2014/07/25 16:33:45, Dirk Pranke wrote:
> On 2014/07/25 05:14:35, shyamp wrote:
> > Done.
>
> Seems fine. I've lost track, though: what's left for you to do before we think
> this is ready and all the feedback has been incorporated?
To Summarize Current Design
1. To enable versioned results pass --enable-versioned-results flag.
If both --enable-versioned-results and --clobber-old-results are passed former
overrides the latter and a warning message is displayed "Flag
--enable_versioned_results overrides --clobber-old-results."
2. The old results folder "layout-test-results" is renamed with a new name
layout-test-results+timestamp(%Y-%m-%d-%H-%M-%S)
3. The script generate_results_dashboard.py parses all the failing_list.json in
the current directory and creates a new json file archived_results.json inside
layout_results_directory
archived_results.json = archived_results + old_results_links
4. Webkit/LayoutTests/fast/harness/dashboard.html is copied to the
layout-test-results directory
5. Added a link from 'results.html' to the dashboard.html in the toolbar
All the feedback has been incorporated ....
The CQ bit was checked by behara.ms@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/behara.ms@samsung.com/339623002/160001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
> To Summarize Current Design > All the feedback has been incorporated .... Okay, I'll take another pass over the whole thing tomorrow, then. Thanks!
On 2014/07/28 12:48:05, patro wrote: > > All the feedback has been incorporated .... Okay, I'll take another look at the whole thing tomorrow, then. Thanks!
lgtm. Please fix the nits below, if possible, but otherwise I think this looks okay to land. Thanks for working on this and being so patient! https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:156: timestamp = time.strftime("%Y-%m-%d-%H-%M-%S", time.gmtime(self._filesystem.mtime(self._filesystem.join(self._results_directory, "results.html")))) Nit: can this be time.localtime() instead of time.gmtime() ? It's a bit confusing to use GMT timestamps for directory names in the local filesystem. https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:191: # FIXME : Add a condition here to limit the number of archived results (total_archived_results < MAX_ARCHIVE_RESULTS) We definitely need to implement this FIXME before this feature is enabled by default (which I'd like to do). https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/process_json_data_unittest.py (right): https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/process_json_data_unittest.py:38: valid_json_data = json.loads('''{ Nit: is there any particular reason to use json for this, rather than just using python data objects directly? I would also prefer it if we didn't need to indent the objects so far ... https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:80: #Generate Dashboard if "--enable-versioned-results is passed" This comment doesn't really add anything, I'd remove it.
PTAL Thanks Shyam Patro https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:156: timestamp = time.strftime("%Y-%m-%d-%H-%M-%S", time.gmtime(self._filesystem.mtime(self._filesystem.join(self._results_directory, "results.html")))) On 2014/07/29 18:53:06, Dirk Pranke wrote: > Nit: can this be time.localtime() instead of time.gmtime() ? It's a bit > confusing to use GMT timestamps for directory names in the local filesystem. Done. https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:191: # FIXME : Add a condition here to limit the number of archived results (total_archived_results < MAX_ARCHIVE_RESULTS) On 2014/07/29 18:53:06, Dirk Pranke wrote: > We definitely need to implement this FIXME before this feature is enabled by > default (which I'd like to do). If we delete the old results once total_archived_results > MAX_ARCHIVED_RESULTS becomes true it will leave dead links to results.html in the dashboard.html as per current design because the links are statically generated. I will do this as another patch. https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/process_json_data_unittest.py (right): https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/process_json_data_unittest.py:38: valid_json_data = json.loads('''{ On 2014/07/29 18:53:06, Dirk Pranke wrote: > Nit: is there any particular reason to use json for this, rather than just using > python data objects directly? I would also prefer it if we didn't need to indent > the objects so far ... Done. https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/339623002/diff/160001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:80: #Generate Dashboard if "--enable-versioned-results is passed" On 2014/07/29 18:53:06, Dirk Pranke wrote: > This comment doesn't really add anything, I'd remove it. Done.
The CQ bit was checked by behara.ms@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/behara.ms@samsung.com/339623002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/17639) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19607)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
The CQ bit was checked by behara.ms@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/behara.ms@samsung.com/339623002/200001
The CQ bit was unchecked by behara.ms@samsung.com
The CQ bit was checked by behara.ms@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/behara.ms@samsung.com/339623002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19916)
Message was sent while issue was closed.
Change committed as 179332
Message was sent while issue was closed.
jrobbins@google.com changed reviewers: - dpranke@chromioum.org |
