Summary: | Add test_expectations.txt syntax checker to check-webkit-style | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenichi Ishibashi <bashi> | ||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, cjerdonek, commit-queue, eric, hamaji | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Kenichi Ishibashi
2010-08-12 01:09:29 PDT
Created attachment 64193 [details]
Proposed Patch
Comment on attachment 64193 [details]
Proposed Patch
WebKitTools/Scripts/webkitpy/style/checker.py:410
+ elif os.path.basename(file_path) == 'test_expectations.txt':
Please add 'drt_expectations.txt' too.
WebKitTools/Scripts/webkitpy/style/checker.py:450
+ if os.path.basename(file_path) == 'test_expectations.txt':
ditto.
Comment on attachment 64193 [details]
Proposed Patch
Thanks for doing this! I believe this is very useful. Putting r- for nitpicks.
WebKitTools/Scripts/webkitpy/style/checker.py:42
+ from checkers.test_expectations import TestExpectationsChecker
It would be better to put this line above checkers.text as we are usually put #include in alphabetical order in C++ code.
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:40
+ sys.path.append(os.path.abspath(os.path.join(d, '../../layout_tests')))
Cannot we just do like
from webkitpy.layout_tests.layout_package import test_expectations
?
Also, I think it would be nice if you add these imports into webkitpy/style_reference.py
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:48
+ class OptionsMock(object):
Why do we need this? Could you add some comments into this docstring?
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:48
+ class OptionsMock(object):
I think the name "Mock" is usually used in unittests. How about ChromiumOptions?
WebKitTools/Scripts/webkitpy/style/checker.py:450
+ if os.path.basename(file_path) == 'test_expectations.txt':
Can we still warn tabs in test_expectations.txt ?
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:93
+ errors = str(err).split('\n')
Not sure, but we can use str.splitlines?
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:85
+ exp = None
Abbreviation isn't encouraged in WebKit. I'd say expectations.
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:63
+ self._output_regex = re.compile('Line:(\d+)\s*(.+)')
It would be better to use named sub-group. Like,
re.compile('Line:(?P<line>\d+)\s*(?P<message>.+)')
With this, you can write match.group('line') to get \d+
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:96
+ match = self._output_regex.match(err)
s/match/matched/ as match is verb and matched is more consistent with cpp.py
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:92
+ except SyntaxError, err:
As we will use "err" for different value, I'd call this as error_object or something line this.
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:95
+ for err in errors:
I slightly prefer error
WebKitTools/ChangeLog:8
+ Just utiizing layout_tests/layout_package/test_expectations.py for checking
utilizing
CCing people who maintain our style bot and/or are great python gurus. (In reply to comment #2) Kent-san, I didn't realize it. I'll add 'drt_expectations.txt' too. Thank you for the comment! (In reply to comment #3) Hamaji-san, Thank you for your quick and detailed review! Your comment definitely helpful for me. I'll send the revised patch later. > (From update of attachment 64193 [details]) > Thanks for doing this! I believe this is very useful. Putting r- for nitpicks. > > WebKitTools/Scripts/webkitpy/style/checker.py:42 > + from checkers.test_expectations import TestExpectationsChecker > It would be better to put this line above checkers.text as we are usually put #include in alphabetical order in C++ code. > > WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:40 > + sys.path.append(os.path.abspath(os.path.join(d, '../../layout_tests'))) > Cannot we just do like > > from webkitpy.layout_tests.layout_package import test_expectations > > ? > > Also, I think it would be nice if you add these imports into webkitpy/style_reference.py > > WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:48 > + class OptionsMock(object): > Why do we need this? Could you add some comments into this docstring? > > WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:48 > + class OptionsMock(object): > I think the name "Mock" is usually used in unittests. How about ChromiumOptions? > > WebKitTools/Scripts/webkitpy/style/checker.py:450 > + if os.path.basename(file_path) == 'test_expectations.txt': > Can we still warn tabs in test_expectations.txt ? > > WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:93 > + errors = str(err).split('\n') > Not sure, but we can use str.splitlines? > > WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:85 > + exp = None > Abbreviation isn't encouraged in WebKit. I'd say expectations. > > WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:63 > + self._output_regex = re.compile('Line:(\d+)\s*(.+)') > It would be better to use named sub-group. Like, > > re.compile('Line:(?P<line>\d+)\s*(?P<message>.+)') > > With this, you can write match.group('line') to get \d+ > > WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:96 > + match = self._output_regex.match(err) > s/match/matched/ as match is verb and matched is more consistent with cpp.py > > WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:92 > + except SyntaxError, err: > As we will use "err" for different value, I'd call this as error_object or something line this. > > WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:95 > + for err in errors: > I slightly prefer error > > WebKitTools/ChangeLog:8 > + Just utiizing layout_tests/layout_package/test_expectations.py for checking > utilizing Created attachment 64305 [details]
Proposed Patch V1
Comment on attachment 64305 [details]
Proposed Patch V1
Looks great. A few nitpicks:
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py:80
+ # Suppress error messages since they will be reported later.
Suppress error messages of test_expectations module since...
would be clearer
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py:36
+ try:
It would be better to add a comment why we want these lines
(In reply to comment #8) Hamaji-san, Thank you for your review! I'll send revised patch and request cq? soon. Thanks, Created attachment 64308 [details]
Proposed Patch V2
Comment on attachment 64308 [details] Proposed Patch V2 Clearing flags on attachment: 64308 Committed r65308: <http://trac.webkit.org/changeset/65308> All reviewed patches have been landed. Closing bug. |