Bug 38693 – cleanup json_results_generator dependencies so that non-layout-tests can also use it safely

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Dependencies and APIs of json_results_generator.py [1] should be cleaned up and layout-tests specific code should be placed only in json_layout_results_generator.py, so that non-layout-tests can safely/cleanly utilize the JSON generator as well.

For example currently json_results_generator.py is also used for chromium's unit/ui testing and called from outside WebKit (from chromium's test script), but it is making things complicated and error prone.

[1] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py
Related chromium bug: http://crbug.com/41405

[Kinuko Yasuda]

Created attachment 55325 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 55325 [details]
Patch

Why does any of this code need _chromium_svn_base?

Nothing outside of ChromiumPort should ever need to touch _chromium_svn_base.

This is not really a [chromium] bug, as WebKit is about to start using json_results_generator to post layout test results as well.

[Eric Seidel (no email)]

This is definitely not chromium-specific as webkit already uses this code as part of new-run-webkit-tests and soon will use it as part of webkit-patch to be able to post historical buildbot results to the test results sever.

[Chris Jerdonek]

FYI, this patch is not applying because __init__.py is an empty file.

Try adding a single comment line.

[Kinuko Yasuda]

_chromium_svn_base is used to get chromium's code revision only if the script
is executed in a chromium tree.  (to clarify, I reorganized the code but the
logic had been there.)

So I'm going to move/hide the platform dependent part into Ports, but the
existing port package is under layout_tests and seems to include a lot of
layout-tests specific stuff.
Should I make another port directory under test_package?  Or what if I extract
some of basic common part of webkitpy/layout_tests/port and put them under
webkitpy/common/port?

[Kinuko Yasuda]

Created attachment 55343 [details]
=add comment in __init__.py

[Kinuko Yasuda]

btw thanks for fixing the bug summary.  I was a bit unaware of non-chromium usages.

[Chris Jerdonek]

Do we want to support checking in empty files?

[Eric Seidel (no email)]

@chris:  Yes, we should support patches with empty files.

[Kinuko Yasuda]

Created attachment 58101 [details]
Take 2 (removed chromium-specific code)

[WebKit Review Bot]

Attachment 58101 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/Scripts/webkitpy/test_package/json_results_generator_unittest.py:60:  indentation is not a multiple of four  [pep8/E111] [5]
WebKitTools/Scripts/webkitpy/test_package/json_results_generator.py:127:  indentation is not a multiple of four  [pep8/E111] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kinuko Yasuda]

Created attachment 58105 [details]
Take 2.1 (style fix)

[Ojan Vafai]

Comment on attachment 58105 [details]
Take 2.1 (style fix)

Very sorry this took so long to review. Now that I can see it in rietveld it's much easier to see that this is mostly just moving code around. Anyways, r- due to a few minor issues. Otherwise this looks good.
---------------------------------
http://wkrietveld.appspot.com/38693/diff/1/2
File WebKitTools/ChangeLog (right):

http://wkrietveld.appspot.com/38693/diff/1/2#newcode18
WebKitTools/ChangeLog:18: * Scripts/webkitpy/test_package/__init__.py: Added.
I think a better home for these files would be a new directory in "common". For example:
webkitpy/common/test.

http://wkrietveld.appspot.com/38693/diff/1/5
File WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py (right):

http://wkrietveld.appspot.com/38693/diff/1/5#newcode46
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:46: # Note: They need to match with the webkitpy.test_package.test_results.
I think you can do this in the import statement and avoid duplicating this list. Namely:
from webkitpy.test_package.test_results import *

If that doesn't work, I'd rather we just use test_results.PASS instead of PASS in this file.

[Kinuko Yasuda]

Created attachment 59169 [details]
Patch

[Kinuko Yasuda]

Thanks for reviewing!

(In reply to comment #13)
> (From update of attachment 58105 [details])
> http://wkrietveld.appspot.com/38693/diff/1/2
> File WebKitTools/ChangeLog (right):
> 
> http://wkrietveld.appspot.com/38693/diff/1/2#newcode18
> WebKitTools/ChangeLog:18: * Scripts/webkitpy/test_package/__init__.py: Added.
> I think a better home for these files would be a new directory in "common". For example:
> webkitpy/common/test.

Done.

> http://wkrietveld.appspot.com/38693/diff/1/5
> File WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py (right):
> 
> http://wkrietveld.appspot.com/38693/diff/1/5#newcode46
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:46: # Note: They need to match with the webkitpy.test_package.test_results.
> I think you can do this in the import statement and avoid duplicating this list. Namely:
> from webkitpy.test_package.test_results import *
> 
> If that doesn't work, I'd rather we just use test_results.PASS instead of PASS in this file.

Done.

[Adam Barth]

Comment on attachment 59169 [details]
Patch

Mostly an r- because it looks like your unit test is talking to the network, which is bad news bears.

WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:1
 +  #!/usr/bin/env python
We don't need this line.  This file isn't runnable on its own.

WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:83
 +      def __init__(self, builder_name, build_name, build_number,
Wow, that's a lot of arguments.  Maybe we'd be better off with some sort of object?

WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:141
 +      def _get_svn_revision(self, in_directory):
This function shouldn't exist.

WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:147
 +          if os.path.exists(os.path.join(in_directory, '.svn')):
scm.py does this work.

WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:149
 +              output = subprocess.Popen(["svn", "info", "--xml"],
Please don't use subprocess directly.  Use executive.py instead.

WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:151
 +                                        shell=(sys.platform == 'win32'),
I'm not sure we want to use the shell here.

WebKitTools/Scripts/webkitpy/common/test/json_results_generator_unittest.py:45
 +  class TestJSONGeneration(unittest.TestCase):
We usually use Test as a suffix instead of a prefix.

WebKitTools/Scripts/webkitpy/common/test/json_results_generator_unittest.py:42
 +  BUILDER_BASE_URL = "http://build.chromium.org/buildbot/gtest-results/"
Is this really going to talk to the network?  We should use a Mock network instead so the unit tests are self-contained.

WebKitTools/Scripts/webkitpy/common/test/json_results_generator_unittest.py:47
 +          self.results_directory = tempfile.mkdtemp()
We should also abstract away access to the file system so we don't need to touch the disk to test this code (which isn't really about manipulating the disk).

WebKitTools/Scripts/webkitpy/common/test/test_results.py:30
 +  # Test results and modifier constants.
I don't understand what role this file places.

[Kinuko Yasuda]

Created attachment 59952 [details]
Patch

[Kinuko Yasuda]

Created attachment 59954 [details]
Patch

[Kinuko Yasuda]

Updated the patch with some additional refactoring changes.
(This time the patch doesn't include directory move/rename as combining update+move generally makes the patch hard to read)

(In reply to comment #16)
> (From update of attachment 59169 [details])
> Mostly an r- because it looks like your unit test is talking to the network, which is bad news bears.
> 
> WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:1
>  +  #!/usr/bin/env python
> We don't need this line.  This file isn't runnable on its own.

Removed.

> WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:83
>  +      def __init__(self, builder_name, build_name, build_number,
> Wow, that's a lot of arguments.  Maybe we'd be better off with some sort of object?

Refactored a bit to use a new TestResult class.

> WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:141
>  +      def _get_svn_revision(self, in_directory):
> This function shouldn't exist.
> 
> WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:147
>  +          if os.path.exists(os.path.join(in_directory, '.svn')):
> scm.py does this work.
> 
> WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:149
>  +              output = subprocess.Popen(["svn", "info", "--xml"],
> Please don't use subprocess directly.  Use executive.py instead.
> 
> WebKitTools/Scripts/webkitpy/common/test/json_results_generator.py:151
>  +                                        shell=(sys.platform == 'win32'),
> I'm not sure we want to use the shell here.

For now I've put an additional comment to explain why it is kept as is.
(I've once replaced this part (_get_svn_revision()) to use scm.py but it caused some run-time
errors on chromium Windows buildbot.  I've been failing to reproduce it on my env for now.)

> WebKitTools/Scripts/webkitpy/common/test/json_results_generator_unittest.py:45
>  +  class TestJSONGeneration(unittest.TestCase):
> We usually use Test as a suffix instead of a prefix.

Fixed.

> WebKitTools/Scripts/webkitpy/common/test/json_results_generator_unittest.py:42
>  +  BUILDER_BASE_URL = "http://build.chromium.org/buildbot/gtest-results/"
> Is this really going to talk to the network?  We should use a Mock network instead so the unit tests are self-contained.

Fixed.  It won't talk to the network.

> WebKitTools/Scripts/webkitpy/common/test/json_results_generator_unittest.py:47
>  +          self.results_directory = tempfile.mkdtemp()
> We should also abstract away access to the file system so we don't need to touch the disk to test this code (which isn't really about manipulating the disk).

Fixed.  It won't touch the disk either.

> WebKitTools/Scripts/webkitpy/common/test/test_results.py:30
>  +  # Test results and modifier constants.
> I don't understand what role this file places.

Removed the file as there wasn't a strong reason to expose those constants there.

[Ojan Vafai]

Comment on attachment 59954 [details]
Patch

Thanks for following up with this. Please fix the nits below before committing.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:190
 +          PASS_RESULT, NO_DATA_RESULT and etc) that indicates the test result
nit: Common style for this would be the following (no "and"):
PASS_RESULT, NO_DATA_RESULT, etc)

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:451
 +  # Please keep the interface until the other script is cleaned up.
Nit: This should probably be phrased as a FIXME. Something like:
# FIXME: Remove this interface once the other script is cleaned up.

Also, a link pointing to the other script (e.g. in src.chromium.org) wouldn't hurt.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:42
 +  class JSONGenerationTest(unittest.TestCase):
How about calling this JSONResultsGeneratorTest?

[Kinuko Yasuda]

Committed r62989: <http://trac.webkit.org/changeset/62989>

[Kinuko Yasuda]

Thanks for your review, submitted with fixes for the nits.

(In reply to comment #20)
> (From update of attachment 59954 [details])
> Thanks for following up with this. Please fix the nits below before committing.
> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:190
>  +          PASS_RESULT, NO_DATA_RESULT and etc) that indicates the test result
> nit: Common style for this would be the following (no "and"):
> PASS_RESULT, NO_DATA_RESULT, etc)
> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:451
>  +  # Please keep the interface until the other script is cleaned up.
> Nit: This should probably be phrased as a FIXME. Something like:
> # FIXME: Remove this interface once the other script is cleaned up.
> 
> Also, a link pointing to the other script (e.g. in src.chromium.org) wouldn't hurt.
> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:42
>  +  class JSONGenerationTest(unittest.TestCase):
> How about calling this JSONResultsGeneratorTest?