Bug 30645 – REGRESSION: 2 failures in run-webkit-unittests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

======================================================================
FAIL: test_braces (modules.cpp_style_unittest.WebKitStyleTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/modules/cpp_style_unittest.py", line 3309, in test_braces
    'Code inside a namespace should not be indented.  [whitespace/indent] [4]')
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/modules/cpp_style_unittest.py", line 219, in assert_multi_line_lint
    self.assertEquals(expected_message, self.perform_multi_line_lint(code, file_name))
AssertionError: 'Code inside a namespace should not be indented.  [whitespace/indent] [4]' != ''

======================================================================
FAIL: test_indentation (modules.cpp_style_unittest.WebKitStyleTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/modules/cpp_style_unittest.py", line 2823, in test_indentation
    'foo.h')
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/modules/cpp_style_unittest.py", line 219, in assert_multi_line_lint
    self.assertEquals(expected_message, self.perform_multi_line_lint(code, file_name))
AssertionError: 'Code inside a namespace should not be indented.  [whitespace/indent] [4]' != ''

----------------------------------------------------------------------
Ran 166 tests in 135.229s

FAILED (failures=2)

Probably caused from http://trac.webkit.org/changeset/49690

[Carol Szabo]

Created attachment 41614 [details]
Proposed Patch

Added back some spaces that were missing from test cases and fixed the expected result in one other.
I do not know how these test cases got in there.

[Eric Seidel (no email)]

I thought that the new style was not to indent code inside namespaces?

[Darin Adler]

Comment on attachment 41614 [details]
Proposed Patch

>          self.assert_multi_line_lint(
>              'namespace WebCore {\n'
>              '#if 0\n'
> -            'class Document {\n'
> +            '    class Document {\n'
>              '};\n'

Really? Indent the line starting the class definition but not the line ending it?

[Carol Szabo]

(In reply to comment #2)
> I thought that the new style was not to indent code inside namespaces?

Yes, you are right. And that is what the cpp_style verifies.

[Carol Szabo]

(In reply to comment #3)
> (From update of attachment 41614 [details])
> >          self.assert_multi_line_lint(
> >              'namespace WebCore {\n'
> >              '#if 0\n'
> > -            'class Document {\n'
> > +            '    class Document {\n'
> >              '};\n'
> 
> Really? Indent the line starting the class definition but not the line ending
> it?

This code has bad style, but currently we do not have a style checker that is able to check indentation other than for the following cases:
1. First level inside a namespace (should be 0).
2. Indentation should be a multiple of 4 columns.
The purpose of the test above was to verify that we do not complain about the indentation of the inner class.

[Carol Szabo]

(In reply to comment #3)
> (From update of attachment 41614 [details])
> >          self.assert_multi_line_lint(
> >              'namespace WebCore {\n'
> >              '#if 0\n'
> > -            'class Document {\n'
> > +            '    class Document {\n'
> >              '};\n'
> 
> Really? Indent the line starting the class definition but not the line ending
> it?

I take my previous comment back. The correct style as I read the style document is not to indent the class definition. This is bad style and the checker should complain.
I agree that this patch is hard to review because it does not give enough context. The next line in the file says that the checker should complain about the indented class definition.

[WebKit Commit Bot]

Comment on attachment 41614 [details]
Proposed Patch

Clearing flags on attachment: 41614

Committed r50091: <http://trac.webkit.org/changeset/50091>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.