Bug 29798 – Move message-port-multi.js from resources to script-tests

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

[Shinichiro Hamaji]

While I was working for Bug 25880, some tests which weren't generated from TEMPLATE.html were added into resources directories.

fast/js/resources/getOwnPropertyDescriptor.js
fast/backgrounds/repeat/resources/margin-shorthand.js
fast/events/resources/message-port-multi.js

The fist two JS requires custom HTMLs so that we don't need to move them (they should have been added in exception lists of make-script-tests-wrappers). It seems we may want to move the last JS into script-tests directory.

[Shinichiro Hamaji]

Created attachment 40220 [details]
Patch v1

[WebKit Commit Bot]

Comment on attachment 40220 [details]
Patch v1

Clearing flags on attachment: 40220

Committed r48823: <http://trac.webkit.org/changeset/48823>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andrew Wilson]

This test contains asynchronous tests, so including js-test-post.js is incorrect.

Note that if you look at the test expections, it inserts "TEST COMPLETE" in the middle of the test output, which is not correct.

[Shinichiro Hamaji]

(In reply to comment #4)
> This test contains asynchronous tests, so including js-test-post.js is
> incorrect.
> 
> Note that if you look at the test expections, it inserts "TEST COMPLETE" in the
> middle of the test output, which is not correct.

I see. So, we can just revert my patch?

[Eric Seidel (no email)]

Yes, it's a known issue that the js testing framework stuff doesn't handle async tests well.  I thought I filed a bug about this a while back.  Basically we'd like to eventually add an "imDone()" method of some sort that would print the TEST COMPLETED stuff for you.  Many tests have already "hacked" this by removing js-test-post.js and done their own TEST COMPLETED writing.  Eventually we should just make this function part of js-test-pre.js and have js-test-post call it when the test is a waitUntilDone() test.

[Andrew Wilson]

It sounds like you were trying to get this test to be generated from
TEMPLATE.html, but it's probably not appropriate for this test since it doesn't
follow the standard form.

I don't actually understand what the motivation behind the test was - if it was
just an attempt at cleanup, then yeah, I'd revert it.

[Shinichiro Hamaji]

Created attachment 40309 [details]
Revert patch v1

[Eric Seidel (no email)]

I think we're trying to slowly improve the "standard form" over time.  The original dream of all the .js tests was that we wouldn't check in the .html files at all.  I'm not sure we'll ever reach that dream though.  Definitely js-test-pre.js and js-test-post.js need losts of love to support more types of testing (like async tests, XHTML tests, SVG tests, etc.)

[Shinichiro Hamaji]

Created attachment 40342 [details]
Revert patch v2

[Shinichiro Hamaji]

(In reply to comment #10)
> Created an attachment (id=40342) [details]
> Revert patch v2

I've updated the revert patch so it has a comment to mention we don't use js-test-post.

Sorry for confusion by this bug.

[WebKit Commit Bot]

Comment on attachment 40342 [details]
Revert patch v2

Rejecting patch 40342 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11355 test cases.
fast/events/message-port-multi.html -> failed

Exiting early after 1 failures. 6051 tests run.
110.37s total testing time

6050 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
2 test cases (<1%) had stderr output

[Shinichiro Hamaji]

Created attachment 40346 [details]
Revert patch v3

[Shinichiro Hamaji]

(In reply to comment #13)
> Created an attachment (id=40346) [details]
> Revert patch v3

Sorry, I used patch -R and resources/*.js wasn't tracked by git. I created the patch again using git mv.

[WebKit Commit Bot]

Comment on attachment 40346 [details]
Revert patch v3

Clearing flags on attachment: 40346

Committed r48926: <http://trac.webkit.org/changeset/48926>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.