Bug 65986 – [Qt] autotests shouldn't require config.h in the style check

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

[Pierre Rossi]

It seems for the most part, our autotests fit pretty wel in the category "consumers of the WebKit API", so they should probably undergo the appropriate style checks.

[Pierre Rossi]

Created attachment 103494 [details]
Patch

[Robert Hogan]

Comment on attachment 103494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103494&action=review

> Tools/ChangeLog:7
> +        This essentially prevents Qt tests from being style-checked
> +        against the same rules that apply to WebCore (e.g. config.h).

I think it's better to have them style-checked myself.

[Pierre Rossi]

(In reply to comment #2)
> (From update of attachment 103494 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103494&action=review
> 
> > Tools/ChangeLog:7
> > +        This essentially prevents Qt tests from being style-checked
> > +        against the same rules that apply to WebCore (e.g. config.h).
> 
> I think it's better to have them style-checked myself.

Hi Robert, I'm not sure to follow you. The point of this change is to allow things like what Benjamin suggested in https://bugs.webkit.org/show_bug.cgi?id=65237 In the case of these tests (apart for the special case of mime type sniffing), we are in the position of being consumers of the API provided by QtWebKit, so it seems to me that rules that enforce including config.h don't belong here. Maybe I should rephrase the ChangeLog message.

[Pierre Rossi]

Created attachment 103596 [details]
Patch

[Benjamin Poulain]

Comment on attachment 103596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103596&action=review

You must also modify the tests accordingly. And you should add a new test for Source/WebKit/qt/tests/MIMESniffing.

> Tools/Scripts/webkitpy/style/checker.py:137
> +    ([# The MIMESniffing test tests the code directly rather than the API,
> +      # let's enable the include check for this one.
> +      "Source/WebKit/qt/tests/MIMESniffing"],
> +     ["+build/include"]),

This list is order sensitive. Only the first path substring match is used. -> Source/WebKit/qt/tests/MIMESniffing will never match

[Pierre Rossi]

Created attachment 103614 [details]
Patch

Benjamin: oops ! looks like I didn't read the comment up there properly.

[Benjamin Poulain]

Comment on attachment 103614 [details]
Patch

Good idea for a patch. I am glad those warning will disappear.

[WebKit Review Bot]

Comment on attachment 103614 [details]
Patch

Clearing flags on attachment: 103614

Committed r92847: <http://trac.webkit.org/changeset/92847>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

Reverted r92847 for reason:

Broke check-webkit-style

Committed r92857: <http://trac.webkit.org/changeset/92857>

[Dimitri Glazkov (Google)]

Sorry guys, this broke the style elf, which is kind of a big deal -- I rolled it out.

[Pierre Rossi]

Created attachment 103646 [details]
Patch

Sorry again about that guys. Since Benjamin mentioned the warnings happened in other places, I figured I'd add the WebKit2 qt tests in this.

[Robert Hogan]

(In reply to comment #3)
> Hi Robert, I'm not sure to follow you. The point of this change is to allow things like what Benjamin suggested in https://bugs.webkit.org/show_bug.cgi?id=65237 In the case of these tests (apart for the special case of mime type sniffing), we are in the position of being consumers of the API provided by QtWebKit, so it seems to me that rules that enforce including config.h don't belong here. Maybe I should rephrase the ChangeLog message.

What I meant was: "The style check keeps the code clean, which is a good thing." If noise like the warnings about config.h are annoying, maybe we should target those instead.

[Andreas Kling]

Comment on attachment 103646 [details]
Patch

rs=me

[Pierre Rossi]

Comment on attachment 103646 [details]
Patch

Let's just hold it for now. I still need to check if that's not messing with alphabetical sorting thingy that was brought up earlier.

[Csaba Osztrogonác]

Is this bug still valid?

[Pierre Rossi]

(In reply to comment #16)
> Is this bug still valid?

Yes, it still is AFAICT, but it's more of a "nice to have" kind of thing though, and I never got back to it. I think I remember the concern raised by Robert turned out to be right and a rule specific to config.h should be introduced and only that should be disabled for autotests.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.