Bug 30640

Summary:

Combine local and session storage tests into one and use script-tests properly

Product:

WebKit

Reporter:

Jeremy Orlow <jorlow>

Component:

New Bugs

Assignee:

Jeremy Orlow <jorlow>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, dglazkov, ojan

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

30535

Attachments:

Description	Flags
Patch v1	none
Patch v1	none
Patch v1	darin: review+

Jeremy Orlow

Reported 2009-10-21 11:27:28 PDT

In https://bugs.webkit.org/show_bug.cgi?id=30535 Darin pointed out that I'm not using "script-tests" correctly. Apparently the tool assumes that all layout tests that correspond to a test in a script-tests directory is under its control. The dom storage layout tests require a small modification to the .html in order to have session storage and local storage tests share the same JavaScript file. I think the best solution for now is to move them back to a resources directory. Darin has suggested that maybe we shouldn't have seprate tests for session storage and local storage, but I'm not positive whether this is the best route to take yet since there are some places where session and local storage diverge (like in quota support).

Attachments
Patch v1 (22.20 KB, patch) 2009-10-21 11:30 PDT, Jeremy Orlow	no flags	Details Formatted Diff Diff
Patch v1 (62.95 KB, patch) 2009-10-21 13:46 PDT, Jeremy Orlow	no flags	Details Formatted Diff Diff
Patch v1 (60.91 KB, patch) 2009-10-21 13:48 PDT, Jeremy Orlow	darin: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Jeremy Orlow

Comment 1 2009-10-21 11:30:26 PDT

Created attachment 41590 [details] Patch v1

Ojan Vafai

Comment 2 2009-10-21 11:55:47 PDT

Why doesn't using a TEMPLATE.html file in domstorage/script-tests work for this?

Jeremy Orlow

Comment 3 2009-10-21 12:10:56 PDT

(In reply to comment #2) > Why doesn't using a TEMPLATE.html file in domstorage/script-tests work for > this? I have no idea. Where's documentation on how to use this script-tests stuff?

Jeremy Orlow

Comment 4 2009-10-21 12:14:27 PDT

(In reply to comment #3) > (In reply to comment #2) > > Why doesn't using a TEMPLATE.html file in domstorage/script-tests work for > > this? > > I have no idea. > > Where's documentation on how to use this script-tests stuff? Oh...well if the template is tied to the script-tests directory it's not going to work because the customization is passing in either localStorage or sessionStorage. If the template is tied to the directory where the HTML exists then we might be able to make this work. The thing is that the directory structure is as follows: domstorage/script-tests domstorage/localstorage domstorage/sessionstorage The latter 2 directories are where the .html files exist. The html files pass in either "localStorage" or "sessionStorage" to the runTest function in the .js files. I suspect that this is different enough from the rest of the script tests that we can't trivially re-use the infrastructure. And, unless this is going to be useful elsewhere, I don't see a point to adapting the infrastructure to this use case.

Jeremy Orlow

Comment 5 2009-10-21 13:46:22 PDT

After talking to Ojan, it sounded like it was best to simply do what Darin originally suggested.

Jeremy Orlow

Comment 6 2009-10-21 13:46:56 PDT

Created attachment 41607 [details] Patch v1

Jeremy Orlow

Comment 7 2009-10-21 13:48:39 PDT

Created attachment 41608 [details] Patch v1

Ojan Vafai

Comment 8 2009-10-21 15:34:15 PDT

One nit: In your template, <script src="../../fast/js/resources/js-test-post-function.js"></script> should go after the runTest call. Is there a problem with that?

Darin Adler

Comment 9 2009-10-21 15:35:39 PDT

Comment on attachment 41608 [details] Patch v1 Looks good. r=me

Jeremy Orlow

Comment 10 2009-10-21 15:43:01 PDT

Ojan: I (In reply to comment #8) > One nit: > > In your template, <script > src="../../fast/js/resources/js-test-post-function.js"></script> should go > after the runTest call. Is there a problem with that? This won't work since the runTest call ends up calling it before the parser will load that file.

Ojan Vafai

Comment 11 2009-10-21 15:56:08 PDT

(In reply to comment #10) > Ojan: I (In reply to comment #8) > > One nit: > > > > In your template, <script > > src="../../fast/js/resources/js-test-post-function.js"></script> should go > > after the runTest call. Is there a problem with that? > > This won't work since the runTest call ends up calling it before the parser > will load that file. That seems fine. Now that you don't need to pass in the storage type from the TEMPLATE.html file, you could reduce the boilerplate needed for new storage JS tests, and be more consistent with most other JS tests. There's no need to have a runTest function at all. Just run the actual JS at the end of the test and set window.successfullyParsed = true. Then instead of using js-test-post-function.js in the TEMPLATE.html file, use js-test-post.js. <html> <head> <link rel="stylesheet" href="../../fast/js/resources/js-test-style.css"> <script src="../../fast/js/resources/js-test-pre.js"></script> </head> <body> <p id="description"></p> <div id="console"></div> <script src="YOUR_JS_FILE_HERE"></script> <script src="../../fast/js/resources/js-test-post.js"></script> </body> </html>

Jeremy Orlow

Comment 12 2009-10-21 16:43:45 PDT

(In reply to comment #11) > There's no need to have a runTest function at all. Just run the actual JS at > the end of the test I've changed my code to do it this way. > and set window.successfullyParsed = true. Then instead of > using js-test-post-function.js in the TEMPLATE.html file, use js-test-post.js. This is not possible since half the local storage tests involve events. This is why I need to use js-test-post-function. But I need to include it first because the other half don't and make the call before the page is fully loaded.

Jeremy Orlow

Comment 13 2009-10-21 16:48:00 PDT

Submitted because this is blocking my other change, but Ojan let me know if you have any further suggestions and I'll address them in future patches.

Ojan Vafai

Comment 14 2009-10-21 18:11:35 PDT

(In reply to comment #12) > (In reply to comment #11) > > and set window.successfullyParsed = true. Then instead of > > using js-test-post-function.js in the TEMPLATE.html file, use js-test-post.js. > > This is not possible since half the local storage tests involve events. This > is why I need to use js-test-post-function. But I need to include it first > because the other half don't and make the call before the page is fully loaded. Ah. I looked at the patch for cases like this and didn't see any. That makes sense then. Thanks for humoring my nits.

Jeremy Orlow

Comment 15 2009-10-21 18:21:20 PDT

(In reply to comment #14) > (In reply to comment #12) > > (In reply to comment #11) > > > and set window.successfullyParsed = true. Then instead of > > > using js-test-post-function.js in the TEMPLATE.html file, use js-test-post.js. > > > > This is not possible since half the local storage tests involve events. This > > is why I need to use js-test-post-function. But I need to include it first > > because the other half don't and make the call before the page is fully loaded. > > Ah. I looked at the patch for cases like this and didn't see any. That makes > sense then. Thanks for humoring my nits. The change that needs it is the one that depends on this. :-)

Jeremy Orlow

Comment 16 2009-10-21 18:21:46 PDT

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #12) > > > (In reply to comment #11) > > > > and set window.successfullyParsed = true. Then instead of > > > > using js-test-post-function.js in the TEMPLATE.html file, use js-test-post.js. > > > > > > This is not possible since half the local storage tests involve events. This > > > is why I need to use js-test-post-function. But I need to include it first > > > because the other half don't and make the call before the page is fully loaded. > > > > Ah. I looked at the patch for cases like this and didn't see any. That makes > > sense then. Thanks for humoring my nits. > > The change that needs it is the one that depends on this. :-) Er...the _test_ that needs this is in the _bug_ that depends on this.

Note You need to log in before you can comment on or make changes to this bug.