Bug 32833 – [check-webkit-style] qt unit testing false positives

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Barth]

Please find below an explanation why the style errors detected by the review bot are either invalid, or cannot be addressed.
Some of these explanations may be wrong, this is why I copied Simon and Laszlo so that they have a chance of correcting me.
 
Thanks for your time and attention,
Carol Szabo
 
 
 
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:21:  Found other
header before WebCore config.h. Should be: config.h, primary header, blank
line, and then alphabetically sorted.  [build/include_order] [4]
 
This first issue I believe to be false. Since the test is not part of WebKit it should not include config.h, I believe that it should not even have access to it since the test is a Qt application and config.h is/should not be, part of the QtWebKit Api

WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:26:  Alphabetical
sorting problem.  [build/include_order] [4]
This issue I believe also to be false, but I may stand corrected. I thought that system headers need to be separated for WebKit headers by an empty line and follow them.
I consider <QDebug>, <QNetworkReply> and <QtTest> system headers since they belong to the platform (Qt), but I may stand corrected since QtWebKit is also currently, part of Qt, hence all headers included in this file could be considered "system" as indicated by the angled brackets, but confused by the fact that the headers do not follow the same naming convention, leading me to treat them differently.
 
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:30:  Should have
a space between // and comment  [whitespace/comments] [4]
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:31:  Should have
a space between // and comment  [whitespace/comments] [4]
 
//TESTED_CLASS=QWebSecurityOrigin
//TESTED_FILES=WebKit/qt/Api/qwebsecurityorigin.*
 
The two comments above appear to be special case comments with some meaning to a tool. I copied them verbatim and have updated them from a different test. Please someone confirm whether it is safe to insert the space after //
 
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:42:  create_data
is incorrectly named. Don't use underscores in your identifier names. 
[readability/naming] [4]
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:44: 
whiteList_data is incorrectly named. Don't use underscores in your identifier
names.  [readability/naming] [4]
Sorry, this cannot be helped. Using tesname_data as the name of the function providing the data for the test is a QtTest framework requirement unlikely to change so the names of these functions will have to stay like this.
 
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:251:  Found
header this file implements after other header. Should be: config.h, primary
header, blank line, and then alphabetically sorted.  [build/include_order] [4]
#include "tst_qwebsecurityorigin.moc"
 
This is actually an error in detection the header name.
This is a pattern required by Qt for files that use moc but have no matching header file and it is used also in other tests and in QtLauncher/main.cpp.

[Laszlo Gombos]

The fundamental issue here is whether API tests - that are not part of WebKit binary per say - needs to also follow the WebKit style or not. It might make sense not to expose WebKit style guide for API tests.

[Kenneth Rohde Christiansen]

(In reply to comment #1)
> The fundamental issue here is whether API tests - that are not part of WebKit
> binary per say - needs to also follow the WebKit style or not. It might make
> sense not to expose WebKit style guide for API tests.

I think it would be better to disable some of the tests for certain subdirs, such as WebKit/qt/Api

[Adam Barth]

Created attachment 46769 [details]
Patch (requires blocking bug to be fixed)

[WebKit Commit Bot]

Comment on attachment 46769 [details]
Patch (requires blocking bug to be fixed)

Clearing flags on attachment: 46769

Committed r53386: <http://trac.webkit.org/changeset/53386>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.