Summary: | cleanup json_results_generator dependencies so that non-layout-tests can also use it safely | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kinuko Yasuda <kinuko> | ||||||||||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, cjerdonek, dpranke, eric, ojan, tony, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | 39665 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Kinuko Yasuda
2010-05-06 14:41:28 PDT
Created attachment 55325 [details]
Patch
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.
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. FYI, this patch is not applying because __init__.py is an empty file. Try adding a single comment line. _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? Created attachment 55343 [details]
=add comment in __init__.py
btw thanks for fixing the bug summary. I was a bit unaware of non-chromium usages. Do we want to support checking in empty files? @chris: Yes, we should support patches with empty files. Created attachment 58101 [details]
Take 2 (removed chromium-specific code)
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.
Created attachment 58105 [details]
Take 2.1 (style fix)
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. Created attachment 59169 [details]
Patch
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. 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. Created attachment 59952 [details]
Patch
Created attachment 59954 [details]
Patch
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. 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?
Committed r62989: <http://trac.webkit.org/changeset/62989> 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? |