Open Bug 1382598 Opened 7 years ago Updated 2 years ago

Mochitest coverage for the storage functionality - review

Categories

(Core :: Storage: Quota Manager, enhancement, P2)

enhancement

Tracking

()

People

(Reporter: icrisan, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

Please review the added coverage.
Attached patch storage_test_coverage (obsolete) — Splinter Review
Run the tests locally and they pass.
Component: Integration: Mercurial → Storage
Product: MozReview → Toolkit
Component: Storage → DOM: Quota Manager
Product: Toolkit → Core
Correct the assignee.
Assignee: ttung → icrisan
Hi Ioana,

I'm afraid that I cannot review your patches. You need to send them to a Storage peer. However, I believe that I can give your feedback to accelerate the review process before your sending your patch to review.
So, before digging into your patch, I'd like to point out two things.

First of all, please follow the format (e.g. indentation) of other existing tests [1]. Moreover, we don't want to have trailing white space into our patch, and please remove them. If you are not sure about what to do, you could check this[2] out.

Secondly, If you want to put your tests under dom/quota, Jan is working on a new internal API (SimpleDB) in Bug 1361330. Rather than using IndexedDB to store data into the disk, I'd suggest using SimpleDB to store data into disk. 

For how to use this API, you could reference the dom/quota/test/unit/test_persist_groupLimit.js insides Bug 1361330 attachment 8882357 [details] [diff] [review].
Note: Bug 1361330 haven't been landed yet, so you need to apply patches there to your code base, and then you could use SimpleDB.

[1] http://searchfox.org/mozilla-central/source/dom/quota/test/browser_permissionsPrompt.html
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
Yeah, I can help with browser_* test first.
Comment on attachment 8888259 [details] [diff] [review]
storage_test_coverage

Review of attachment 8888259 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your contribution, a lot of works. This is very initial feedback. I will check again after issues/questions addressed.

::: dom/quota/test/badEstimate1Bug1374970.html
@@ +12,5 @@
> +			// Set the global limit to 50MB
> +			SpecialPowers.pushPrefEnv({
> +				"set": [["dom.quotaManager.temporaryStorage.fixedLimit", 50000]]
> +			}, continueToNextStep);
> +			yield undefined;

Calling pushPrefEnv once is enough.

@@ +28,5 @@
> +				testGenerator.next(result.quota);
> +			})
> +			let groupLimit = yield undefined;
> +
> +			// Create an ArrayBuffer with the size of (groupLimit / 3)

Can you explain why you need to do "groupLimit / 3"?

::: dom/quota/test/badEstimate2Bug1374970.html
@@ +14,5 @@
> +				testGenerator.next(result.quota);
> +			})
> +			let groupLimit = yield undefined;
> +
> +			// Create an ArrayBuffer with the size of the (groupLimit / 2)  

Why do you need to do 'groupLimit /2'? Do you mind explaining it?

::: dom/quota/test/badEstimateBug1374970.js
@@ +25,5 @@
> +	info("Loading test page: " + testPageURL);
> +	gBrowser.selectedBrowser.loadURI(testPageURL);
> +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> +
> +	// Assert that the estimate() method returns the usage per origin and not per entire group 

trailing space

::: dom/quota/test/browserHelpers.js
@@ +47,5 @@
>      testGenerator.next();
>    }, 0);
>  }
> +
> +function openDB(dbname, objectStoreName) 

nit: trailing space

@@ +50,5 @@
> +
> +function openDB(dbname, objectStoreName) 
> +{
> +  return new Promise(function (resolve, reject) {
> +  let openRequest = indexedDB.open(dbname);

nit: fix indentation

::: dom/quota/test/browser_globalLimitValidation.js
@@ +93,5 @@
> +	info("Loading test page: " + testPageURL);
> +	gBrowser.selectedBrowser.loadURI(testPageURL);
> +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> +
> +	await waitForMessage('cleanup', gBrowser);

This doesn't seem to validate anything. Is it necessary to create a page to do cleanup?

::: dom/quota/test/browser_globalLimitValidation1.html
@@ +12,5 @@
> +      // Set the global limit to 50MB
> +      SpecialPowers.pushPrefEnv({
> +        "set": [["dom.quotaManager.temporaryStorage.fixedLimit", 50000]]
> +      }, continueToNextStep);
> +      yield undefined;

Calling pushPrefEnv once is enough.

@@ +20,5 @@
> +        ["dom.storageManager.prompt.testing", true],
> +        ["dom.storageManager.prompt.testing.allow", true],
> +        ["browser.storageManager.enabled", true]]
> +      }, continueToNextStep);
> +      yield undefined;

Maybe you can move SpecialPowers.pushPrefEnv() section to browser_globalLimitValidation.js? Since each browser_*.html use the same prefs.

@@ +22,5 @@
> +        ["browser.storageManager.enabled", true]]
> +      }, continueToNextStep);
> +      yield undefined;
> +
> +      // Get the tab's quota(groupLimit) 

trailing space

@@ +31,5 @@
> +
> +      navigator.storage.persist().then((result) => {
> +        testGenerator.next(result);
> +      })
> +      let persistResult = yield undefined;

persistResult doesn't seem to be used.

::: dom/quota/test/browser_globalLimitValidation2.html
@@ +2,5 @@
> +  Any copyright is dedicated to the Public Domain.
> +  http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<html>
> + 

nit:trailing space. I won't point it out the rest of places.

::: dom/quota/test/browser_globalLimitValidation7.html
@@ +2,5 @@
> +  Any copyright is dedicated to the Public Domain.
> +  http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<html>
> +

I'm not sure creating this page is necessary, maybe I missed something. Do you mind explaining the reason?

::: dom/quota/test/browser_privateWindowEstimate.js
@@ +13,5 @@
> +	win.gBrowser.selectedTab = win.gBrowser.addTab();
> +
> +	info("Loading test page: " + url);
> +	win.gBrowser.selectedBrowser.loadURI(url);
> +	await waitForMessage(10485760, win.gBrowser);

Just want to confirm that 10485760 constant value is the same even on different platform? Will it change on different platforms? Comparing this constant value seems to be dangerous. Since we run tests on different platforms.

::: dom/quota/test/browser_privateWindowPersisted.js
@@ +17,5 @@
> +	win.gBrowser.selectedBrowser.loadURI(testPageURL);
> +	await waitForMessage(false, win.gBrowser);
> +
> +	win.gBrowser.removeCurrentTab();
> +	win.close();

Please add one line after win.close():
win = null;
(In reply to Tom Tung [:tt] from comment #5)

Hi Tom,

Thank you very much for your feedback. I will resolve the indentation and the trailing white spaces. Also I will change the way I store data into DB I will use simpleDB.
(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> Yeah, I can help with browser_* test first.

Hi Shawn,

Thank you very much for your review! I will reply your comment #7 after I will check the hard-coded values(e.g: "10485760" on all the platforms)
(In reply to Ioana Crisan from comment #9)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> > Yeah, I can help with browser_* test first.
> 
> Hi Shawn,
> 
> Thank you very much for your review! I will reply your comment #7 after I
> will check the hard-coded values(e.g: "10485760" on all the platforms)

Sorry, I did not explain the supported platforms very well.
See: https://mozilla-releng.net/trychooser/
This is try-server command generator. You can check which platform you need to test with.
Since there are many different platforms and I guess you can push your code to try server.
(In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> Comment on attachment 8888259 [details] [diff] [review]
> storage_test_coverage
> 
> Review of attachment 8888259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for your contribution, a lot of works. This is very initial feedback.
> I will check again after issues/questions addressed.
> 
> ::: dom/quota/test/badEstimate1Bug1374970.html
> @@ +12,5 @@
> > +			// Set the global limit to 50MB
> > +			SpecialPowers.pushPrefEnv({
> > +				"set": [["dom.quotaManager.temporaryStorage.fixedLimit", 50000]]
> > +			}, continueToNextStep);
> > +			yield undefined;
> 
> Calling pushPrefEnv once is enough.

Indeed, I will call the pushPrefEnv once.

> 
> @@ +28,5 @@
> > +				testGenerator.next(result.quota);
> > +			})
> > +			let groupLimit = yield undefined;
> > +
> > +			// Create an ArrayBuffer with the size of (groupLimit / 3)
> 
> Can you explain why you need to do "groupLimit / 3"?

In this test(badEstimate1Bug1374970.html) I wanted to test that estimate() method returns the usage per origin and not per entire group. 
The 2 sites used(belonging to the same group) would aggregately use a maximum of 20% of the global limit(one group limit). 
I started from setting the fixedLimit pref to 50MB, in this way the group limit will be 10MB(10,485,760 bytes).
Because both sites can aggregatly use 10MB(1,048,576 bytes) I tried to store different amount of data in each one to better differentiate what the estimate() method returns; creating a blob with the size of groupLimit/2 respectively groupLimit/3 is just an example, which summed up does not exceed the quota limit.
Because the estimate() method is run from the second html(where the blob stored is equal to groupLimit/2) I expected a quota of 5308544 bytes instead of 8869461 bytes(groupLimit/2 + groupLimit/3). This test is written based on the bug #1374970 

NOTE: This test is currently skipped until the bug is fixed

 
> ::: dom/quota/test/badEstimate2Bug1374970.html
> @@ +14,5 @@
> > +				testGenerator.next(result.quota);
> > +			})
> > +			let groupLimit = yield undefined;
> > +
> > +			// Create an ArrayBuffer with the size of the (groupLimit / 2)  
> 
> Why do you need to do 'groupLimit /2'? Do you mind explaining it?

Please see the above answer. Thanks!
 
> ::: dom/quota/test/badEstimateBug1374970.js
> @@ +25,5 @@
> > +	info("Loading test page: " + testPageURL);
> > +	gBrowser.selectedBrowser.loadURI(testPageURL);
> > +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> > +
> > +	// Assert that the estimate() method returns the usage per origin and not per entire group 
> 
> trailing space

I will fix the trailing spaces and also the indentation. Thanks!

> ::: dom/quota/test/browserHelpers.js
> @@ +47,5 @@
> >      testGenerator.next();
> >    }, 0);
> >  }
> > +
> > +function openDB(dbname, objectStoreName) 
> 
> nit: trailing space

I will fix the trailing spaces and also the indentation. Thanks!

> @@ +50,5 @@
> > +
> > +function openDB(dbname, objectStoreName) 
> > +{
> > +  return new Promise(function (resolve, reject) {
> > +  let openRequest = indexedDB.open(dbname);
> 
> nit: fix indentation

I will fix the trailing spaces and also the indentation. Thanks!

> ::: dom/quota/test/browser_globalLimitValidation.js
> @@ +93,5 @@
> > +	info("Loading test page: " + testPageURL);
> > +	gBrowser.selectedBrowser.loadURI(testPageURL);
> > +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> > +
> > +	await waitForMessage('cleanup', gBrowser);
> 
> This doesn't seem to validate anything. Is it necessary to create a page to
> do cleanup?

Yes, it is necessary, because otherwise the 'persistent-storage' permission is removed before the cleanup of the db takes place, meaning that the persisted data will not be affected by the cleanup, this will result in failing of the succeeded tests. So this is used to clear the db after the 'persistent-storage' permission is removed.
 
> ::: dom/quota/test/browser_globalLimitValidation1.html
> @@ +12,5 @@
> > +      // Set the global limit to 50MB
> > +      SpecialPowers.pushPrefEnv({
> > +        "set": [["dom.quotaManager.temporaryStorage.fixedLimit", 50000]]
> > +      }, continueToNextStep);
> > +      yield undefined;
> 
> Calling pushPrefEnv once is enough.

Indeed, I will call the pushPrefEnv once.

> @@ +20,5 @@
> > +        ["dom.storageManager.prompt.testing", true],
> > +        ["dom.storageManager.prompt.testing.allow", true],
> > +        ["browser.storageManager.enabled", true]]
> > +      }, continueToNextStep);
> > +      yield undefined;
> 
> Maybe you can move SpecialPowers.pushPrefEnv() section to
> browser_globalLimitValidation.js? Since each browser_*.html use the same
> prefs.

Yes, that seems to be working, should I do that for all the tests or just this one ?

> @@ +22,5 @@
> > +        ["browser.storageManager.enabled", true]]
> > +      }, continueToNextStep);
> > +      yield undefined;
> > +
> > +      // Get the tab's quota(groupLimit) 
> 
> trailing space

I will fix the trailing spaces and also the indentation. Thanks! :)

> @@ +31,5 @@
> > +
> > +      navigator.storage.persist().then((result) => {
> > +        testGenerator.next(result);
> > +      })
> > +      let persistResult = yield undefined;
> 
> persistResult doesn't seem to be used.

Yes, I will remove it.

> ::: dom/quota/test/browser_globalLimitValidation2.html
> @@ +2,5 @@
> > +  Any copyright is dedicated to the Public Domain.
> > +  http://creativecommons.org/publicdomain/zero/1.0/
> > +-->
> > +<html>
> > + 
> 
> nit:trailing space. I won't point it out the rest of places.

I will fix the trailing spaces and also the indentation. :)

> ::: dom/quota/test/browser_globalLimitValidation7.html
> @@ +2,5 @@
> > +  Any copyright is dedicated to the Public Domain.
> > +  http://creativecommons.org/publicdomain/zero/1.0/
> > +-->
> > +<html>
> > +
> 
> I'm not sure creating this page is necessary, maybe I missed something. Do
> you mind explaining the reason?

I responded to this to your comment above, it's necessary to do a proper cleanup of the db after the 'persistent-storage' permission has been removed. 

> ::: dom/quota/test/browser_privateWindowEstimate.js
> @@ +13,5 @@
> > +	win.gBrowser.selectedTab = win.gBrowser.addTab();
> > +
> > +	info("Loading test page: " + url);
> > +	win.gBrowser.selectedBrowser.loadURI(url);
> > +	await waitForMessage(10485760, win.gBrowser);
> 
> Just want to confirm that 10485760 constant value is the same even on
> different platform? Will it change on different platforms? Comparing this
> constant value seems to be dangerous. Since we run tests on different
> platforms.

Andrei will check this using the try-server you provided and will let us know. Thanks!

> ::: dom/quota/test/browser_privateWindowPersisted.js
> @@ +17,5 @@
> > +	win.gBrowser.selectedBrowser.loadURI(testPageURL);
> > +	await waitForMessage(false, win.gBrowser);
> > +
> > +	win.gBrowser.removeCurrentTab();
> > +	win.close();
> 
> Please add one line after win.close():
> win = null;

Sure, I will do.

Thanks a lot for the review!
> > ::: dom/quota/test/browser_globalLimitValidation.js
> > @@ +93,5 @@
> > > +	info("Loading test page: " + testPageURL);
> > > +	gBrowser.selectedBrowser.loadURI(testPageURL);
> > > +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> > > +
> > > +	await waitForMessage('cleanup', gBrowser);
> > 
> > This doesn't seem to validate anything. Is it necessary to create a page to
> > do cleanup?
> 
> Yes, it is necessary, because otherwise the 'persistent-storage' permission
> is removed before the cleanup of the db takes place, meaning that the
> persisted data will not be affected by the cleanup, this will result in
> failing of the succeeded tests. So this is used to clear the db after the
> 'persistent-storage' permission is removed.
>  
Maybe we can add finishTest() when onunload in browser_globalLimitValidation6.html 
<body onload="continueToNextStep()" onunload="finishTest()">
Then we don't need to create browser_globalLimitValidation7.html to do finishTest().

And 'persistent-storage' permission is for Storage API (which exposed to content page) not for qms.clearStoragesForPrincipal.
So calling qms.clearStoragesForPrincipal will clear the whole folder no matter it's persisted or best-effort.
(In reply to Shawn Huang [:shawnjohnjr] from comment #12)
> > > ::: dom/quota/test/browser_globalLimitValidation.js
> > > @@ +93,5 @@
> > > > +	info("Loading test page: " + testPageURL);
> > > > +	gBrowser.selectedBrowser.loadURI(testPageURL);
> > > > +	await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> > > > +
> > > > +	await waitForMessage('cleanup', gBrowser);
> > > 
> > > This doesn't seem to validate anything. Is it necessary to create a page to
> > > do cleanup?
> > 
> > Yes, it is necessary, because otherwise the 'persistent-storage' permission
> > is removed before the cleanup of the db takes place, meaning that the
> > persisted data will not be affected by the cleanup, this will result in
> > failing of the succeeded tests. So this is used to clear the db after the
> > 'persistent-storage' permission is removed.
> >  
> Maybe we can add finishTest() when onunload in
> browser_globalLimitValidation6.html 
> <body onload="continueToNextStep()" onunload="finishTest()">
> Then we don't need to create browser_globalLimitValidation7.html to do
> finishTest().

I tried what you mention at the beginning but it doesn’t work that way, because when finishTest() will be called on onunload the site will still be persisted, meaning that the db won’t be cleared for the next test.

> And 'persistent-storage' permission is for Storage API (which exposed to
> content page) not for qms.clearStoragesForPrincipal.
> So calling qms.clearStoragesForPrincipal will clear the whole folder no
> matter it's persisted or best-effort.
Hi Ioana,

In today storage meeting with Jan, we discuss this. 

First of all, he wants to say thank you for doing this. :)
Also, you don't need to rewrite your patch to use SimpleDB, because it's an internal API and it's for simplifying complicated tests.
Flags: needinfo?(icrisan)
I ran the current patch through Try at Andrei's request. In the future, don't hesitate to needinfo me if you need help running your patch through Try until you get your SSH issues sorted out.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c4a6399e2df6c1d86074034d26eca9b2581a1a6
Looks like there's timeouts and debug assertions in the tests. The orange and red results in the previous link that don't have stars next to them show the issues that need attention. Also, please make sure the tests have trailing newlines at the end.
I also see this warning in the logs:
WARNING - Warning: badEstimateBug1374970.js from manifest /home/worker/workspace/build/tests/mochitest/browser/dom/quota/test/browser.ini is not a valid test
(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> I ran the current patch through Try at Andrei's request. In the future,
> don't hesitate to needinfo me if you need help running your patch through
> Try until you get your SSH issues sorted out.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5c4a6399e2df6c1d86074034d26eca9b2581a1a6

Thank you for running my patch through Try.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Looks like there's timeouts and debug assertions in the tests. The orange
> and red results in the previous link that don't have stars next to them show
> the issues that need attention. Also, please make sure the tests have
> trailing newlines at the end.

I will investigate the failing tests. I also created a new branch where I fixed the indentation, trailing spaces and the other small changes that Shawn requested me.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> I also see this warning in the logs:
> WARNING - Warning: badEstimateBug1374970.js from manifest
> /home/worker/workspace/build/tests/mochitest/browser/dom/quota/test/browser.
> ini is not a valid test

The badEstimateBug1374970.js is a test created for bug #1374970. It is not fixed yet and the test is skipped. If you want I can remove the test until the bug is fixed.
Attachment #8888259 - Attachment is obsolete: true
Flags: needinfo?(icrisan)
Hi Ryan,
Could you please run the newly patch through Try?
Thank you!
Flags: needinfo?(ryanvm)
Comment on attachment 8901804 [details] [diff] [review]
storage_test_coverage_2

Looks like this fixed the timeouts from the first patch :). A few comments:
1.) Looks like there's still a lot of missing newlines at the end of these files. Please make sure to add them (or configure your editor to do so automatically).
2.) Try to maintain alphabetical ordering in browser.ini instead of just adding the new tests to the end.
3.) The debug failures (like https://treeherder.mozilla.org/logviewer.html#?job_id=126449819&repo=try) are likely a real bug in Gecko code that will need attention from the developers of this feature.

Nice job, these are looking good!
Attachment #8901804 - Flags: feedback+
Depends on: 1394745
No longer depends on: 1394745
Depends on: 1394745
Created a patch(storage_test_coverage_3) and added newlines at the end of each file and also ordered alphabetically the tests in browser.ini. Patch will be run through Try after bug #1394745 is fixed.
I saw the patch title already came with r=shuang at the end. Unfortunately I'm not a peer to rubber-stamp patches. I commented patches because I just want to reduce unnecessary back-and-forth.
(In reply to Shawn Huang [:shawnjohnjr] from comment #26)
> I saw the patch title already came with r=shuang at the end. Unfortunately
> I'm not a peer to rubber-stamp patches. I commented patches because I just
> want to reduce unnecessary back-and-forth.

Hi, Shawn
Thank you for helping me with the review! Could you please point me the person in charge for doing this?
Thank you!
I think you can ask Jan first. Let's see how we can continue this review process.
Flags: needinfo?(jvarga)
Yeah, I'll take a look at this, but maybe some time next week. I have many other reviews to do which waited for long time.
Flags: needinfo?(jvarga)
Attachment #8902236 - Flags: review?(jvarga)
Hi Ioana,

There is still a failure [1] after applying the fix in bug 1394745, but I think it's not related to the issue in bug 1394745. 
It looks like you need to handle the QuotaExceedError exception in the test.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=127050039&repo=try&lineNumber=32316
(In reply to Tom Tung [:tt] from comment #30)
> Hi Ioana,
> 
> There is still a failure [1] after applying the fix in bug 1394745, but I
> think it's not related to the issue in bug 1394745. 
> It looks like you need to handle the QuotaExceedError exception in the test.
> 
> [1]
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=127050039&repo=try&lineNumber=32316

Hi Tom,

This happens only on Try. I cannot reproduce it on my desktop (ubuntu 16.04) :(

The failed test is the end to end scenario from the Etherpad: https://public.etherpad-mozilla.org/p/New_storage linked below:

[Steps]
Step 1. Open three sites in the different tabs. 
Step 2. Persist one of them. (e.g. persist a.com)
Step 3. Make Firefox reach the global limit by keeping storing data into that three sites.
Step 4. Close the tab to the other two sites. (e.g. close b.com & c.com)
Step 5. Try to write data into the persisted site, and it should work.
Step 6. Close the tab of persisted site and open the two sites which have been closed. (e.g. close a.com and open b.com & c.com)
Step 7. If you try to write more data into them, it should fail.

I didn't handle the "QuotaExceedError" exception when I added the first blob in the 3rd html because I didn't expect this to be triggered. I handled the exception when I added the second blob. 

I will investigate a bit more this issue.
(In reply to Ioana Crisan from comment #31)
> (In reply to Tom Tung [:tt] from comment #30) 
> This happens only on Try. I cannot reproduce it on my desktop (ubuntu 16.04)

BTW, the failure only happens on Window 7 debug build based in the try push [1]. Hope that hleps.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b39ee585cc5f8ef2a4fa26ec14513f3ae805bbcb&selectedJob=127050039
Which may be pointing at non-e10s mode as the issue. Try running the tests locally with --disable-e10s set and see what happens.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #33)
> Which may be pointing at non-e10s mode as the issue. Try running the tests
> locally with --disable-e10s set and see what happens.

If I run the tests locally without "--disable-e10s" I get the following results:

148 INFO Browser Chrome Test Summary
149 INFO Passed:  39
150 INFO Failed:  0
151 INFO Todo:    0
152 INFO Mode:    e10s


If I run the tests locally with "--disable-e10s" I get the following results:


Browser Chrome Test Summary
	Passed: 39
	Failed: 5
	Todo: 0
	Mode: non-e10s


The following tests failed:
152 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/browser_globalLimitValidation.js | uncaught exception - QuotaExceededError at https://www.itisatrap.org/browser/dom/quota/test/browser_globalLimitValidation3.html:43:6
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
153 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/browser_globalLimitValidation.js | uncaught exception - QuotaExceededError at https://example.org/browser/dom/quota/test/browser_globalLimitValidation5.html:36:6
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
154 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/browser_globalLimitValidation.js | uncaught exception - QuotaExceededError at https://www.itisatrap.org/browser/dom/quota/test/browser_globalLimitValidation6.html:36:6
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
155 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/browser_groupLimitNonPersistedOrigs.js | uncaught exception - QuotaExceededError at https://test2.example.com/browser/dom/quota/test/browser_groupLimitPersistedOrig2.html:51:6
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
156 INFO TEST-UNEXPECTED-FAIL | dom/quota/test/browser_groupLimitPersistedOrig.js | uncaught exception - QuotaExceededError at https://test2.example.com/browser/dom/quota/test/browser_groupLimitPersistedOrig2.html:51:6
Priority: -- → P2
Jan, any chance you can take a look at this at some point?
Flags: needinfo?(jvarga)
Assignee: icrisan → jvarga
Tom, you already know our testing infrastructure and conventions for stuff like this.
Could you please do a pre-review for this, I'll take a look then. Thanks
Flags: needinfo?(jvarga) → needinfo?(shes050117)
Attachment #8902236 - Flags: review?(jvarga)
Will take a look at it tomorrow.
Comment on attachment 8901804 [details] [diff] [review]
storage_test_coverage_2

Review of attachment 8901804 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for submitting tests to verify that!

The testing logic is clear to me, but it seems to me that we could try to make it better. Thus, would you mind changing it a little bit? I'd like to revisit it after the following things are fixed.

First of all, it's better to have small patches for reviews. Thus, could you split this big patch into a few smaller patches which in each patch contain one test inside? For instance, files related to  browser_globalLimitValidation.js in a patch, files related to browser_groupLimitNonPersistedOrigs.js in another patch and so on.

Secondly, for the HTML documents (browser_globalLimitValidation1.html ... browser_globalLimitValidation7.html), could you try to make a generic HTML document rather having 7 files?
For example, you could try to have an HTML file receiving commands (, says "FILL",) from browser_globalLimitValidation.js and executes a set of commands (, in this case basically what you did in browser_globalLimitValidation1.html,). Once it gets QuotaExceedError, post a message [1] to js to let it know what happens.

Thirdly, there are a few coding format's nits. Please try to match the coding format for tests under indexedDB. I comment that in the patch. Please apply them to all the files if it's possible.

Note: please let me know if it's still not clear to you what to do. I'm definitely willing to help you to do that.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage

::: dom/quota/test/browser_globalLimitValidation.js
@@ +1,1 @@
> +add_task(async function testglobalLimitValidation() {

It'd be great if you can provide a few sentences to describe what you want to do and how do you achieve that at the top of the page. It makes people easier to understand what you've done and what's it inside.

Also, would you mind changing the title of this file? IIUC, this test is trying to validate QuotaExceededError is triggered as expected. Perhaps, it's nicer to have a name which is much more related to what it do.

@@ +8,5 @@
> +    ["dom.quotaManager.temporaryStorage.fixedLimit", 50000]]
> +  });
> +
> +  let urls = [
> +    'https://example.com:443/browser/dom/quota/test/browser_globalLimitValidation1.html',

nits: 
\' works for JS string. However, it's better to match coding format for browser tests under indexedDB prefer using \" instead of \' for strings, so, for instance, please change it to "https://example.com:443/browser/dom/quota/test/browser_globalLimitValidation1.html".

Please do that and apply that to all the rest of the strings in all the tests.

::: dom/quota/test/browser_globalLimitValidation1.html
@@ +1,1 @@
> +<!--

It seems to me that it's better to name this document something like browser_fillGroupSpace.html or browser_fillGroupQuota.html or ... etc. What do you think?

@@ +32,5 @@
> +        testGenerator.next(result);
> +      })
> +      let db = yield undefined;
> +
> +      for (let i = 0; i < 3; i++) {

Maybe, create a const variable which is set to 3 so that it's easier to let others understand what does it mean.
for Comment 38
Flags: needinfo?(shes050117) → needinfo?(icrisan)
Hi Tom,

First, thank you for reviewing my patch!
Since my last update for this feature in Bugzilla I have been moved to other project inside Mozilla, so now I contribute for a different testing framework. My new QA lead is Matt Wobensmith(mwobensmith@mozilla.com). Please contact him in order to prioritize this bug on my tasks board.

Thank you, 
Ioana
Flags: needinfo?(icrisan) → needinfo?(shes050117)
(In reply to Ioana Crisan from comment #40)
Thanks for the reply and let me know that! 

Hi Matt,

Would you mind prioritizing this? Thanks!

Ioana wrote these tests and the testing logic is clear, but I want to change them a bit so that they can be a little more succinct before sending them to the final review.
Flags: needinfo?(shes050117) → needinfo?(mwobensmith)
FWIW, my recollection from way back is that there were failures on the Try push too (debug assertions IIRC). You should definitely rebase these and re-run them through Try before going much further.
Actually, Tracy Walker manages Ioana and her work, so I am going to remove myself and add him for a comment.
Flags: needinfo?(mwobensmith) → needinfo?(twalker)
Ioana has moved on to another test development project. We need her on our project so we can hit our fast approaching target release date.  

It looks like the test work here is done with some minor changes sought by Tom Tung.  Since this test work has become priority for Jan and Tom, Tom is welcome to carry the patch to the finish with any changes he sees fit.

Thank you.
Flags: needinfo?(twalker)
Assignee: jvarga → shes050117

I'm actively working on this, so drop the assignee

Assignee: ttung → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: