Bug 34574 – check-webkit-style should ignore underscore in Qt autotests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jędrzej Nowacki]

I believe that check-webkit-style should ignore "Don't use underscores in your identifier names.  [readability/naming] [4]" rule inside Qt's autotests. There is no clean way to avoid a xxx_data methods as they are called automatically by the QtTest Module.

Please ignore the rule inside these sub-folders:
Webkit/qt/tests
JavaScriptCore/qt/tests

[Chris Jerdonek]

Created attachment 48286 [details]
Proposed patch

[Chris Jerdonek]

Created attachment 48287 [details]
Proposed patch 2

Minor fixes to ChangeLog and a code comment.

[Shinichiro Hamaji]

Adding Gustavo in CC list.

My comment about GTK+ isn't related to this bug. Please feel free to land this patch as is (or you may want to modify the list-comprehension), and I'll open another bug to decide path-rules for GTK+.

> @@ -98,6 +106,15 @@ _PATH_RULES_SPECIFIER = [
>        "WebKit/qt/QGVLauncher/"],
>       ("-build/include",
>        "-readability/streams")),
> +    ([# The GTK+ APIs use GTK+ naming style, which includes
> +      # lower-cased, underscore-separated values.
> +      "WebKit/gtk/webkit/",
> +      # There is no clean way to avoid "xxx_data" methods inside
> +      # Qt's autotests since they are called automatically by the
> +      # QtTest module.
> +      "WebKit/qt/tests/",
> +      "JavaScriptCore/qt/tests"],
> +     ("-readability/naming",)),
>      # These are test file patterns.
>      (["_test.cpp",
>        "_unittest.cpp",

Per discussion in Bug 34572, we need to add WebKitTools/DumpRenderTree/gtk. I guess gtk code doesn't care build/include_order (#includes should be alphabetically sorted) and readability/null (don't use NULL, use 0 instead) as well. So, I'd propose

    ([# The GTK+ APIs use GTK+ naming style, which includes
      # lower-cased, underscore-separated values.
      # GTK+ code uses NULL and unsorted #includes, too.
      "WebKit/gtk/webkit/",
      "WebKitTools/DumpRenderTree/gtk",],
     ("-build/include_order",
      "-readability/naming",
      "-readability/null")),
    ([# There is no clean way to avoid "xxx_data" methods inside
      # Qt's autotests since they are called automatically by the
      # QtTest module.
      "WebKit/qt/tests/",
      "JavaScriptCore/qt/tests"],
     ("-readability/naming",)),

or something like this. Gustavo, could you check my guess is correct?

> +    def _get_path_specific_lower(self):
> +        """Return a copy of self._path_specific with the paths lower-cased."""
> +        if self._path_specific_lower is None:
> +            self._path_specific_lower = []
> +            for (sub_paths, path_rules) in self._path_specific:
> +                sub_paths = [sub_path.lower() for sub_path in sub_paths]

Though this is fine as is, I think there's a bit simpler way:

sub_paths = map(str.lower, sub_paths)

[Shinichiro Hamaji]

Probably, for gtk, we may want to disable whitespace/declaration as well? See the review bot's comment in Bug 32170.

[Chris Jerdonek]

Comment on attachment 48287 [details]
Proposed patch 2

cq- to use webkit-patch

[Chris Jerdonek]

Manually committed (via "git svn dcommit"):

http://trac.webkit.org/changeset/54482

[Adam Barth]

any particular reason you removed

- self.assert_lint('void this_is_a_gtk_style_name(int var1, int var2)', '', 'WebKit/gtk/webkit/foo.cpp')

?  Seems like a good test to have...

[Chris Jerdonek]

(In reply to comment #7)
> any particular reason you removed
> 
> - self.assert_lint('void this_is_a_gtk_style_name(int var1, int var2)', '',
> 'WebKit/gtk/webkit/foo.cpp')
> 
> ?  Seems like a good test to have...

Yes.  I removed it because cpp.py is no longer responsible for checking that file path. All of that is now encoded by the _PATH_RULES_SPECIFIER configuration variable and taken care of by the FilterConfiguration class.

I had a similar discussion with Shinichiro at the end of this comment

https://bugs.webkit.org/show_bug.cgi?id=33684#c19

after removing similar hard-coded directory names from cpp.py and their corresponding unit tests.

Testing like the above can now go in the GlobalVariablesTest.test_path_rules_specifier() unit test method.  That method is responsible for checking that the _PATH_RULES_SPECIFIER variable is functioning as expected.

I suppose I could have added another assert to that method for a gtk file path, but there is already an assert in there for a qt file path that tests the same exemption:

>        self.assertFalse(config.should_check("readability/naming",
>                             "WebKit/qt/tests/qwebelement/tst_qwebelement.cpp"))

The exemption is encoded here:

>    ([# The GTK+ APIs use GTK+ naming style, which includes
>      # lower-cased, underscore-separated values.
>      "WebKit/gtk/webkit/",
>      # There is no clean way to avoid "xxx_data" methods inside
>      # Qt's autotests since they are called automatically by the
>      # QtTest module.
>      "WebKit/qt/tests/",
>      "JavaScriptCore/qt/tests"],
>     ["-readability/naming"]),

[Chris Jerdonek]

(In reply to comment #8)
> I suppose I could have added another assert to that method for a gtk file path,
> but there is already an assert in there for a qt file path that tests the same
> exemption:
> 
> >        self.assertFalse(config.should_check("readability/naming",
> >                             "WebKit/qt/tests/qwebelement/tst_qwebelement.cpp"))

I do think it makes sense to have at least one assert per string that appears in _PATH_RULES_SPECIFIER (to have complete code coverage and avoid typos, etc).  I will do that the next time I modify this area of code.

[Chris Jerdonek]

(In reply to comment #7)
> any particular reason you removed
> 
> - self.assert_lint('void this_is_a_gtk_style_name(int var1, int var2)', '',
> 'WebKit/gtk/webkit/foo.cpp')
> 
> ?  Seems like a good test to have...

FYI, as I mentioned in comment 9 of this report, I restored the unit test for a "WebKit/gtk/webkit/" path in the proposed patches of this report:

https://bugs.webkit.org/show_bug.cgi?id=34932

I also added a unit test for every other path that appears in the _PATH_RULES_SPECIFIER configuration variable.

[Jędrzej Nowacki]

It seems that the problem is still valid. Please look at bug 40911.

[Eric Seidel (no email)]

This bug has been waiting to land for 6 months?  Or did it get landed already?

[Adam Barth]

Sadly, cjerdonek seems to have moved on to geener pastures.

[Shinichiro Hamaji]

I think there are no reason we cannot land this patch. I guess Chris put cq- just because this requires Bug 33684 closed. As the patch for Bug 33684 seems to be landed already, I think we should be able to land this patch. I'll try.

[Shinichiro Hamaji]

Comment on attachment 48287 [details]
Proposed patch 2

(In reply to comment #14)
> I think there are no reason we cannot land this patch. I guess Chris put cq- just because this requires Bug 33684 closed. As the patch for Bug 33684 seems to be landed already, I think we should be able to land this patch. I'll try.

Actually, this patch has already been landed: http://trac.webkit.org/changeset/54482

The false-positives in Bug 40911 happened because we don't have qt/benchmark in the exemption list.

[Shinichiro Hamaji]

Filed Bug 51467.