Introducing the syntax checker of test_expectations.txt to check-webkit-style would be helpful to write layout tests. I'll send proposed patch soon.
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.