Bug 43899 – Add test_expectations.txt syntax checker to check-webkit-style

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

Created attachment 64193 [details]
Proposed Patch

[Kent Tamura]

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.

[Shinichiro Hamaji]

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

[Shinichiro Hamaji]

CCing people who maintain our style bot and/or are great python gurus.

[Kenichi Ishibashi]

(In reply to comment #2)
Kent-san,

I didn't realize it. I'll add 'drt_expectations.txt' too. Thank you for the comment!

[Kenichi Ishibashi]

(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

[Kenichi Ishibashi]

Created attachment 64305 [details]
Proposed Patch V1

[Shinichiro Hamaji]

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

[Kenichi Ishibashi]

(In reply to comment #8)
Hamaji-san,

Thank you for your review! I'll send revised patch and request cq? soon.

Thanks,

[Kenichi Ishibashi]

Created attachment 64308 [details]
Proposed Patch V2

[WebKit Commit Bot]

Comment on attachment 64308 [details]
Proposed Patch V2

Clearing flags on attachment: 64308

Committed r65308: <http://trac.webkit.org/changeset/65308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.