32833 2009-12-21 12:04:04 -0800 [check-webkit-style] qt unit testing false positives 2010-01-17 19:12:02 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 33771 1 abarth webkit-unassigned carol commit-queue kenneth laszlo.gombos levin oldest_to_newest 174028 0 abarth 2009-12-21 12:04:04 -0800 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. 174031 1 laszlo.gombos 2009-12-21 12:08:49 -0800 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. 175544 2 kenneth 2009-12-29 07:04:04 -0800 (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 181647 3 46769 abarth 2010-01-17 16:26:23 -0800 Created attachment 46769 Patch (requires blocking bug to be fixed) 181695 4 46769 commit-queue 2010-01-17 19:11:56 -0800 Comment on attachment 46769 Patch (requires blocking bug to be fixed) Clearing flags on attachment: 46769 Committed r53386: <http://trac.webkit.org/changeset/53386> 181696 5 commit-queue 2010-01-17 19:12:02 -0800 All reviewed patches have been landed. Closing bug. 46769 2010-01-17 16:26:23 -0800 2010-01-17 19:11:56 -0800 Patch (requires blocking bug to be fixed) bug-32833-20100117162621.patch text/plain 2245 abarth ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCA5YWM0MWE4Li5hZmFjNjY1IDEwMDY0NAotLS0gYS9XZWJLaXRUb29scy9DaGFuZ2VM b2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0yLDYgKzIsMTggQEAKIAogICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KIAorICAgICAgICBbY2hlY2std2Via2l0LXN0 eWxlXSBxdCB1bml0IHRlc3RpbmcgZmFsc2UgcG9zaXRpdmVzCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMjgzMworCisgICAgICAgIEV4ZW1wdCB0aGUg UXQgQVBJIGFuZCB1bml0IHRlc3RzIGZyb20gdGhlIHN0eWxlIGNoZWNrZXIuCisKKyAgICAgICAg KiBTY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NwcF9zdHlsZS5weToKKyAgICAgICAgKiBTY3JpcHRz L3dlYmtpdHB5L3N0eWxlL2NwcF9zdHlsZV91bml0dGVzdC5weToKKworMjAxMC0wMS0xNyAgQWRh bSBCYXJ0aCAgPGFiYXJ0aEB3ZWJraXQub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCiAgICAgICAgIHN0eWxlLWNoZWNrIHNjcmlwdCByZXBvcnRzIGxvYWRzIG9m IGVycm9ycyBvbiBndGsyZHJhd2luZy5jCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0zMzc3MQogCmRpZmYgLS1naXQgYS9XZWJLaXRUb29scy9TY3JpcHRz L3dlYmtpdHB5L3N0eWxlL2NwcF9zdHlsZS5weSBiL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0 cHkvc3R5bGUvY3BwX3N0eWxlLnB5CmluZGV4IGRmNmE2NTguLjgxZjViZDYgMTAwNjQ0Ci0tLSBh L1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY3BwX3N0eWxlLnB5CisrKyBiL1dl YktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY3BwX3N0eWxlLnB5CkBAIC0yOTM2LDYg KzI5MzYsMTIgQEAgZGVmIGlzX2V4ZW1wdChmaWxlbmFtZSk6CiAgICAgQXJnczoKICAgICAgIGZp bGVuYW1lOiBBIGZpbGVuYW1lLiBJdCBtYXkgY29udGFpbiBkaXJlY3RvcnkgbmFtZXMuCiAgICAg ICIiIgorICAgIGlmIChmaWxlbmFtZS5maW5kKCdXZWJLaXQvcXQvQXBpLycpID49IDAKKyAgICAg ICAgb3IgZmlsZW5hbWUuZmluZCgnV2ViS2l0L3F0L3Rlc3RzLycpID49IDApOgorICAgICAgICAj IFRoZSBRdCBBUEkgYW5kIHRlc3RzIGRvIG5vdCBmb2xsb3cgV2ViS2l0IHN0eWxlLgorICAgICAg ICAjIFRoZXkgZm9sbG93IFF0IHN0eWxlLiA6KQorICAgICAgICByZXR1cm4gVHJ1ZQorCiAgICAg cmV0dXJuIG9zLnBhdGguYmFzZW5hbWUoZmlsZW5hbWUpIGluICgKICAgICAgICAgJ2d0azJkcmF3 aW5nLmMnLAogICAgICAgICAnZ3RrMmRyYXdpbmcuaCcsCmRpZmYgLS1naXQgYS9XZWJLaXRUb29s cy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NwcF9zdHlsZV91bml0dGVzdC5weSBiL1dlYktpdFRv b2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY3BwX3N0eWxlX3VuaXR0ZXN0LnB5CmluZGV4IDMz NzAwYmMuLjBlN2MxMTIgMTAwNjQ0Ci0tLSBhL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkv c3R5bGUvY3BwX3N0eWxlX3VuaXR0ZXN0LnB5CisrKyBiL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Vi a2l0cHkvc3R5bGUvY3BwX3N0eWxlX3VuaXR0ZXN0LnB5CkBAIC0zNTgwLDYgKzM1ODAsOCBAQCBj bGFzcyBXZWJLaXRTdHlsZVRlc3QoQ3BwU3R5bGVUZXN0QmFzZSk6CiAgICAgICAgIHNlbGYuYXNz ZXJ0XyhjcHBfc3R5bGUuaXNfZXhlbXB0KCdXZWJDb3JlL3BsYXRmb3JtL2d0ay9ndGsyZHJhd2lu Zy5oJykpCiAgICAgICAgIHNlbGYuYXNzZXJ0XyhjcHBfc3R5bGUuaXNfZXhlbXB0KCdndGsyZHJh d2luZy5jJykpCiAgICAgICAgIHNlbGYuYXNzZXJ0XyhjcHBfc3R5bGUuaXNfZXhlbXB0KCdXZWJD b3JlL3BsYXRmb3JtL2d0ay9ndGsyZHJhd2luZy5jJykpCisgICAgICAgIHNlbGYuYXNzZXJ0Xyhj cHBfc3R5bGUuaXNfZXhlbXB0KCdXZWJLaXQvcXQvQXBpL3F3ZWJwYWdlLmgnKSkKKyAgICAgICAg c2VsZi5hc3NlcnRfKGNwcF9zdHlsZS5pc19leGVtcHQoJ1dlYktpdC9xdC90ZXN0cy9xd2Vic2Vj dXJpdHlvcmlnaW4vdHN0X3F3ZWJzZWN1cml0eW9yaWdpbi5jcHAnKSkKIAogZGVmIHRlYXJEb3du KCk6CiAgICAgIiIiQSBnbG9iYWwgY2hlY2sgdG8gbWFrZSBzdXJlIGFsbCBlcnJvci1jYXRlZ29y aWVzIGhhdmUgYmVlbiB0ZXN0ZWQuCg==