Bug 86055 - [BlackBerry] [DRT] Ref-tests were not run by torch-launcher
Summary: [BlackBerry] [DRT] Ref-tests were not run by torch-launcher
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-09 20:45 PDT by Xiaobo Wang
Modified: 2013-05-21 20:09 PDT (History)
3 users (show)

See Also:


Attachments
Git patch for bug 86055 (8.04 KB, patch)
2012-05-09 23:21 PDT, Xiaobo Wang
no flags Details | Formatted Diff | Diff
Git patch updated for code style (7.99 KB, patch)
2012-05-09 23:56 PDT, Xiaobo Wang
rwlbuis: review-
Details | Formatted Diff | Diff
Git patch updated for comments from Bob and Ming (24.83 KB, patch)
2012-05-10 23:42 PDT, Xiaobo Wang
no flags Details | Formatted Diff | Diff
Git patch updated for more comments from Rob (24.84 KB, patch)
2012-05-11 05:28 PDT, Xiaobo Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaobo Wang 2012-05-09 20:45:33 PDT
Currently torch-launcher only run tests parsed by NRWT, with ref-tests excluded. As a result, if a test have ref-tests (which were not run), our DumpRenderTree Perl script will think there's a crash and exit with code 1. So NRWT will report the result as CRASH.
We need to update DumpRenderTree.cpp to try to find ref-tests and run them.

Example:
When running test "css2.1/20110323/border-conflict-element-001.htm" we should be able to find its ref-test "css2.1/20110323/border-conflict-element-001-expected.html" and run it as well.
Comment 1 Xiaobo Wang 2012-05-09 23:21:15 PDT
Created attachment 141090 [details]
Git patch for bug 86055
Comment 2 WebKit Review Bot 2012-05-09 23:25:47 PDT
Attachment 141090 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Tools/DumpRenderTree/blackbe..." exit_code: 1
Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:236:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:248:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:259:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Xiaobo Wang 2012-05-09 23:56:12 PDT
Created attachment 141094 [details]
Git patch updated for code style
Comment 4 Rob Buis 2012-05-10 14:39:47 PDT
Comment on attachment 141094 [details]
Git patch updated for code style

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

Looks good, can be cleaned up some more.

> ChangeLog:1
> +2012-05-09  Xiaobo Wang  <xbwang@torchmobile.com.cn>

You need an empty line here.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:206
> +    const char* surfixes[] = {"-expected", "-expected-mismatch"};

Typo, suffix.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:207
> +    const int countofSurfixes = sizeof(surfixes) / sizeof(const char*);

Ditto.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:406
> +    for (Vector<WTF::String>::iterator iter = files.begin(); files.end() != iter; ++iter)

Can you try if it works if you do not add WTF:: in this cpp file?
Comment 5 Ming Xie 2012-05-10 15:59:02 PDT
Xiaobo, aren't you going to rename this?
+    int getRefTests(const WTF::String& testName);

Also, the return value never gets checked.
Comment 6 Xiaobo Wang 2012-05-10 23:42:14 PDT
Created attachment 141333 [details]
Git patch updated for comments from Bob and Ming
Comment 7 Xiaobo Wang 2012-05-10 23:51:03 PDT
Thanks Rob, please forgive my poor spelling and carelessness:)
Without WTF:: it also works, so I cleaned it for all instances in DumpRenderTree.cpp and .h.
Comment 8 Xiaobo Wang 2012-05-11 00:19:31 PDT
(In reply to comment #5)
> Xiaobo, aren't you going to rename this?
> +    int getRefTests(const WTF::String& testName);
> 
> Also, the return value never gets checked.

Thanks Ming. updated to return void.
Comment 9 Xiaobo Wang 2012-05-11 00:20:56 PDT
(In reply to comment #6)
> Created an attachment (id=141333) [details]
> Git patch updated for comments from Bob and Ming

Sorry, I mean Rob not Bob.
Comment 10 Rob Buis 2012-05-11 04:32:12 PDT
Comment on attachment 141333 [details]
Git patch updated for comments from Bob and Ming

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

Looks much better! Please fix remaining issues before landing.

> ChangeLog:8
> +        Ref-tests are tests with surfix "-expected", "-expected-mismatch" and a valid

surfix!

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:209
> +    const char* extensions[] = {".html"};

We may want to add .svg as possible ref-test extension later.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:222
> +

Remove empty line.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:401
> +    // and multiple mismatch ref-tests

Lacks a period at the end.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:403
> +        getRefTests(*iter);

I think it should be getRefTest, not getRefTests.
Comment 11 Rob Buis 2012-05-11 04:32:49 PDT
(In reply to comment #9)
> (In reply to comment #6)
> > Created an attachment (id=141333) [details] [details]
> > Git patch updated for comments from Bob and Ming
> 
> Sorry, I mean Rob not Bob.

No problem :)
Comment 12 Xiaobo Wang 2012-05-11 05:12:06 PDT
(In reply to comment #10)
Again thanks for the good catches!

> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:209
> > +    const char* extensions[] = {".html"};
> 
> We may want to add .svg as possible ref-test extension later.
Great, I'll add ".svg"to the array.
> 

> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:403
> > +        getRefTests(*iter);
> 
> I think it should be getRefTest, not getRefTests.
Why do you think so? A test may have multiple ref-tests.
Comment 13 Xiaobo Wang 2012-05-11 05:28:07 PDT
Created attachment 141382 [details]
Git patch updated for more comments from Rob
Comment 14 Rob Buis 2012-05-11 05:39:17 PDT
Comment on attachment 141382 [details]
Git patch updated for more comments from Rob

Looks good.
Comment 15 WebKit Review Bot 2012-05-11 06:25:00 PDT
Comment on attachment 141382 [details]
Git patch updated for more comments from Rob

Clearing flags on attachment: 141382

Committed r116758: <http://trac.webkit.org/changeset/116758>
Comment 16 Rob Buis 2012-05-12 15:01:07 PDT
Closing, this landed in r116758.
Comment 17 Xiaobo Wang 2013-05-21 20:07:36 PDT
Comment on attachment 141333 [details]
Git patch updated for comments from Bob and Ming

Clear r/cq flags for obsolete patch.
Comment 18 Xiaobo Wang 2013-05-21 20:07:52 PDT
Clear r/cq flags for obsolete patch.
Comment 19 Xiaobo Wang 2013-05-21 20:08:27 PDT
Comment on attachment 141333 [details]
Git patch updated for comments from Bob and Ming

Clear.
Comment 20 Xiaobo Wang 2013-05-21 20:09:13 PDT
close.