Enabling archiving of test results by default in run-webkit-tests.

Enabling archiving of test results by default in run-webkit-tests.

Added unit_tests for archiving of layout test results.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181991

[patro, 2014-08-20 13:45:37]

Kindly Review

Thanks
Shyam Patro

[vivekg, 2014-08-20 14:14:05]

We may need to merge the clobbering patch along with this.

https://codereview.chromium.org/489093002/diff/1/Tools/Scripts/webkitpy/layou...
File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right):

https://codereview.chromium.org/489093002/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:81: gen_dash_board =
GenerateDashBoard(port)
nit: GenerateDashBoard can be renamed to more class sounding name as:
DashBoardGenerator

[Dirk Pranke, 2014-08-20 16:15:19]

https://codereview.chromium.org/489093002/diff/1/Tools/Scripts/webkitpy/layou...
File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right):

https://codereview.chromium.org/489093002/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:176: help="Disable
archiving of test results."),
Just get rid of the flag altogether, and make --clobber-old-results delete
everything (all runs). 

If we have that, then I see no need to be able to turn off archiving. The
difference between keeping 1 run's results around and 10 doesn't matter much.

We can always add it later if we need to.

(i.e., merge this w/ the clobber patch as Vivek suggests ...)

[patro, 2014-08-20 18:51:11]

PTAL

Thanks,
Shyam Patro

https://codereview.chromium.org/489093002/diff/1/Tools/Scripts/webkitpy/layou...
File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right):

https://codereview.chromium.org/489093002/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:176: help="Disable
archiving of test results."),
On 2014/08/20 16:15:19, Dirk Pranke wrote:
> Just get rid of the flag altogether, and make --clobber-old-results delete
> everything (all runs). 
> 
> If we have that, then I see no need to be able to turn off archiving. The
> difference between keeping 1 run's results around and 10 doesn't matter much.
> 
> We can always add it later if we need to.
> 
> (i.e., merge this w/ the clobber patch as Vivek suggests ...)

Done.

[Dirk Pranke, 2014-08-20 19:05:10]

This is just about what I had in mind.

I think there's probably at least one problem in the code (noted below).

Also, a change of this scale should have new tests (and might affect existing
tests), so please make sure to check that and add tests as need be.

https://codereview.chromium.org/489093002/diff/20001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (left):

https://codereview.chromium.org/489093002/diff/20001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:410:
self._port.clobber_old_port_specific_results()
You should probably keep this call as well.

https://codereview.chromium.org/489093002/diff/20001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/489093002/diff/20001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:405:
self._filesystem.rmtree(dir)
I think this will delete *any* directory that exists alongside
layout-test-results, not just the archived results directories.

If I'm right, I don't think we want to be that aggressive :).

[patro, 2014-08-20 19:30:14]

PTAL

Thanks
Shyam Patro

https://codereview.chromium.org/489093002/diff/20001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (left):

https://codereview.chromium.org/489093002/diff/20001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:410:
self._port.clobber_old_port_specific_results()
On 2014/08/20 19:05:10, Dirk Pranke wrote:
> You should probably keep this call as well.

Done.

https://codereview.chromium.org/489093002/diff/20001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/489093002/diff/20001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:405:
self._filesystem.rmtree(dir)
On 2014/08/20 19:05:10, Dirk Pranke wrote:
> I think this will delete *any* directory that exists alongside
> layout-test-results, not just the archived results directories.
> 
> If I'm right, I don't think we want to be that aggressive :).

Assuming that the test results folder name results will start with
"layout-test-results" we can keep a check and filter out only those directories
starting with "layout-test-results" rather than deleting all the directories.

[Dirk Pranke, 2014-08-20 20:09:57]

https://codereview.chromium.org/489093002/diff/40001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/489093002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:176: if
self._filesystem.isdir(file_path) and 'layout-test-results' in dir:
You should use the basename of self._results_directory, like we do in
rename_results_folder(), above, rather than assuming a hardcoded string.

https://codereview.chromium.org/489093002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:401: if
self._filesystem.isdir(file_path) and 'layout-test-results' in dir:
same comment.

In fact, you should probably split the logic out into separate functions,
perhaps one that figures out which directories to delete and another that
actually doese the deleting of a list of dirs.

[patro, 2014-08-21 13:40:39]

PTAL

Thanks,
Shyam Patro

https://codereview.chromium.org/489093002/diff/40001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/489093002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:176: if
self._filesystem.isdir(file_path) and 'layout-test-results' in dir:
On 2014/08/20 20:09:57, Dirk Pranke wrote:
> You should use the basename of self._results_directory, like we do in
> rename_results_folder(), above, rather than assuming a hardcoded string.

Done.

https://codereview.chromium.org/489093002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:401: if
self._filesystem.isdir(file_path) and 'layout-test-results' in dir:
On 2014/08/20 20:09:57, Dirk Pranke wrote:
> same comment.
> 
> In fact, you should probably split the logic out into separate functions,
> perhaps one that figures out which directories to delete and another that
> actually doese the deleting of a list of dirs.

Done.

https://codereview.chromium.org/489093002/diff/40001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:401: if
self._filesystem.isdir(file_path) and 'layout-test-results' in dir:
On 2014/08/20 20:09:57, Dirk Pranke wrote:
> same comment.
> 
> In fact, you should probably split the logic out into separate functions,
> perhaps one that figures out which directories to delete and another that
> actually doese the deleting of a list of dirs.

Done.

[Dirk Pranke, 2014-08-21 15:52:46]

Looks pretty good. Still needs some sort of test to make sure we're archiving by
default and pruning by default when we need to.

[patro, 2014-08-22 18:35:25]

On 2014/08/21 15:52:46, Dirk Pranke wrote:
> Looks pretty good. Still needs some sort of test to make sure we're archiving
by
> default and pruning by default when we need to.

To write unit_tests for archiving of results some fixes are required in
MockFileSystem

Adding them as a separate patches
https://codereview.chromium.org/493413002/
https://codereview.chromium.org/500453002/

Kindly Review

Thanks,
Shyam Patro

[Dirk Pranke, 2014-08-27 23:47:54]

On 2014/08/22 18:35:25, patro wrote:
> On 2014/08/21 15:52:46, Dirk Pranke wrote:
> > Looks pretty good. Still needs some sort of test to make sure we're
archiving
> by
> > default and pruning by default when we need to.
> 
> To write unit_tests for archiving of results some fixes are required in
> MockFileSystem
> 
> Adding them as a separate patches
> https://codereview.chromium.org/493413002/
> https://codereview.chromium.org/500453002/
> 
> Kindly Review

I've reviewed them now (sorry for the delay, I was on vacation for a few days
...). You'll land those patches and then update this CL w/ tests, right?

-- Dirk

[patro, 2014-09-05 12:12:11]

behara.ms@samsung.com changed reviewers:
+ kphanee@chromium.org

[patro, 2014-09-05 12:12:12]

Added unit_tests

PTAL

Thanks,
Shyam Patro

[Dirk Pranke, 2014-09-05 21:07:31]

This is now pretty close, just a few minor comments.

Also, please update the description of the change; currently you describing
adding a flag that we're not actually adding.

https://codereview.chromium.org/489093002/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/489093002/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:400:
results_directory_path = self._filesystem.dirname(self._results_directory)
Looking at this again, I'd probably call this something like
'dir_above_results_path' ; 'results_directory_path' seems like it would be the
same thing as _results_directory.

https://codereview.chromium.org/489093002/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right):

https://codereview.chromium.org/489093002/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:125: return
I don't think we should catch this error. It shouldn't normally happen, and a
debug message is just going to be ignored. We should let the exception
propagate.

[patro, 2014-09-06 06:50:55]

PTAL 

Thanks,
Shyam Patro

https://codereview.chromium.org/489093002/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/489093002/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:400:
results_directory_path = self._filesystem.dirname(self._results_directory)
On 2014/09/05 21:07:31, Dirk Pranke wrote:
> Looking at this again, I'd probably call this something like
> 'dir_above_results_path' ; 'results_directory_path' seems like it would be the
> same thing as _results_directory.

Done.

https://codereview.chromium.org/489093002/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right):

https://codereview.chromium.org/489093002/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:125: return
On 2014/09/05 21:07:31, Dirk Pranke wrote:
> I don't think we should catch this error. It shouldn't normally happen, and a
> debug message is just going to be ignored. We should let the exception
> propagate.

Done.

[Dirk Pranke, 2014-09-06 16:53:01]

lgtm.

We should fix the issues noted in the comments, below, but since they weren't
introduced in this patch, you could either do them as part of this patch or
separately.

https://codereview.chromium.org/489093002/diff/140001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py (right):

https://codereview.chromium.org/489093002/diff/140001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:122:
input_json_string = file.readline()
This seems like a bug. I don't know why we'd only want to read the first line of
the file rather than the whole file, and given that this is JSONP and the
ADD_RESULTS() call wraps the entire json expression, it seems like it would
break if we had multi-line JSON.

I think we're getting away with it because the JSON is only a single (very long)
line.

Thus, you could change this to just 'input_json_string =
self._filesystem.read_text_file(current_failing_results_json_file)'

https://codereview.chromium.org/489093002/diff/140001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:139: with
self._filesystem.open_text_file_for_reading(full_json_file_path) as full_file:
same comments ...

https://codereview.chromium.org/489093002/diff/140001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:154:
file.write(final_json)
similar comment: I'm not sure why we would't just call
self._filesystem.write_text_file(...) and skip the 'with'.

[patro, 2014-09-06 17:41:57]

On 2014/09/06 16:53:01, Dirk Pranke wrote:
> lgtm.
> 
> We should fix the issues noted in the comments, below, but since they weren't
> introduced in this patch, you could either do them as part of this patch or
> separately.
> 
>
https://codereview.chromium.org/489093002/diff/140001/Tools/Scripts/webkitpy/...
> File Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py
(right):
> 
>
https://codereview.chromium.org/489093002/diff/140001/Tools/Scripts/webkitpy/...
> Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:122:
> input_json_string = file.readline()
> This seems like a bug. I don't know why we'd only want to read the first line
of
> the file rather than the whole file, and given that this is JSONP and the
> ADD_RESULTS() call wraps the entire json expression, it seems like it would
> break if we had multi-line JSON.
> 
> I think we're getting away with it because the JSON is only a single (very
long)
> line.
> 
> Thus, you could change this to just 'input_json_string =
> self._filesystem.read_text_file(current_failing_results_json_file)'
> 
>
https://codereview.chromium.org/489093002/diff/140001/Tools/Scripts/webkitpy/...
> Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:139: with
> self._filesystem.open_text_file_for_reading(full_json_file_path) as full_file:
> same comments ...
> 
>
https://codereview.chromium.org/489093002/diff/140001/Tools/Scripts/webkitpy/...
> Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py:154:
> file.write(final_json)
> similar comment: I'm not sure why we would't just call
> self._filesystem.write_text_file(...) and skip the 'with'.

I will fix them in a separate patch.

[patro, 2014-09-06 17:42:12]

The CQ bit was checked by behara.ms@samsung.com

[commit-bot: I haz the power, 2014-09-06 17:42:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/behara.ms@samsung.com/489093002/140001

[commit-bot: I haz the power, 2014-09-06 18:00:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-06 18:00:10]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[patro, 2014-09-07 06:05:52]

Patchset #9 (id:160001) has been deleted

[patro, 2014-09-07 06:19:41]

The CQ bit was checked by behara.ms@samsung.com

[commit-bot: I haz the power, 2014-09-07 06:20:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/behara.ms@samsung.com/489093002/180001

[commit-bot: I haz the power, 2014-09-07 07:27:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-07 07:27:10]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[patro, 2014-09-15 11:55:02]

Fixed trybot failures 

Kindly Review

Thanks,
Shyam Patro

[Dirk Pranke, 2014-09-15 16:45:25]

lgtm

[patro, 2014-09-15 17:47:54]

The CQ bit was checked by behara.ms@samsung.com

[commit-bot: I haz the power, 2014-09-15 17:49:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/489093002/200001

[commit-bot: I haz the power, 2014-09-15 18:02:37]

Committed patchset #10 (id:200001) as 181991