[Webkit-unassigned] [Bug 32833] New: [check-webkit-style] qt unit testing false positives

[bugzilla-daemon at webkit.org]

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

           Summary: [check-webkit-style] qt unit testing false positives
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: Tools / Tests
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: abarth at webkit.org
                CC: levin at chromium.org, laszlo.1.gombos at nokia.com,
                    carol.szabo at nokia.com


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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.