Bug 32833 - [check-webkit-style] qt unit testing false positives
Summary: [check-webkit-style] qt unit testing false positives
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 33771
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-21 12:04 PST by Adam Barth
Modified: 2010-01-17 19:12 PST (History)
5 users (show)

See Also:


Attachments
Patch (requires blocking bug to be fixed) (2.19 KB, patch)
2010-01-17 16:26 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-12-21 12:04:04 PST
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.
Comment 1 Laszlo Gombos 2009-12-21 12:08:49 PST
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.
Comment 2 Kenneth Rohde Christiansen 2009-12-29 07:04:04 PST
(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
Comment 3 Adam Barth 2010-01-17 16:26:23 PST
Created attachment 46769 [details]
Patch (requires blocking bug to be fixed)
Comment 4 WebKit Commit Bot 2010-01-17 19:11:56 PST
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>
Comment 5 WebKit Commit Bot 2010-01-17 19:12:02 PST
All reviewed patches have been landed.  Closing bug.