Bug 86055 – [BlackBerry] [DRT] Ref-tests were not run by torch-launcher

• Status: CLOSED
• Resolution: FIXED
• Component: Tools / Tests

[Xiaobo Wang]

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.

[Xiaobo Wang]

Created attachment 141090 [details]
Git patch for bug 86055

[WebKit Review Bot]

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.

[Xiaobo Wang]

Created attachment 141094 [details]
Git patch updated for code style

[Rob Buis]

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
> [phone]  Xiaobo Wang  <[e-mail]>

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?

[Ming Xie]

Xiaobo, aren't you going to rename this?
+    int getRefTests(const WTF::String& testName);

Also, the return value never gets checked.

[Xiaobo Wang]

Created attachment 141333 [details]
Git patch updated for comments from Bob and Ming

[Xiaobo Wang]

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.

[Xiaobo Wang]

(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.

[Xiaobo Wang]

(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.

[Rob Buis]

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.

[Rob Buis]

(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 :)

[Xiaobo Wang]

(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.

[Xiaobo Wang]

Created attachment 141382 [details]
Git patch updated for more comments from Rob

[Rob Buis]

Comment on attachment 141382 [details]
Git patch updated for more comments from Rob

Looks good.

[WebKit Review Bot]

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>

[Rob Buis]

Closing, this landed in r116758.

[Xiaobo Wang]

Comment on attachment 141333 [details]
Git patch updated for comments from Bob and Ming

Clear r/cq flags for obsolete patch.

[Xiaobo Wang]

Clear r/cq flags for obsolete patch.

[Xiaobo Wang]

Comment on attachment 141333 [details]
Git patch updated for comments from Bob and Ming

Clear.

[Xiaobo Wang]

close.