1389378 - Test to verify QM creates originInfo when the temporary storage has been initialized

Status: RESOLVED FIXED

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

Description

[Tom Tung [:tt, :ttung]]

We decide to move the test in the bug 1372116 to this bug in the storage meeting since this shouldn't block the Storage API v1. 

Note: This test is written via SimpleDB API.

Comment 1

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - P1: Test to verify persist() do create originInfo when the temporary storage has been initialized.

Comment 2

[Marion Daly [:mdaly]]

Janv, can you take a look at this?

Comment 3

[Jan Varga [:janv]]

Yes.

Comment 4

[Tom Tung [:tt, :ttung]]

Rewriting this test, and I'm considering to move this patch to bug 1389390 since they both use simpleDB and might use the same function

Comment 5

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

I changed my mind and decided to keep the patch here because the test is much simpler than I expected.

Note that setTemporaryLimit() is assumed to be added to dom/quota/test/unit/head.js in bug 1389390.

Comment 6

[Tom Tung [:tt, :ttung]]

Comment on attachment 9010560 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Jan, this patch is a test for verifying persisted origins are bounded by the global limit.

Comment 7

[Jan Varga [:janv]]

Yeah, we also another test - test_persist_groupLimit.js, I already created it locally, will file a bug for that.

Comment 8

[Jan Varga [:janv]]

s/we also another test/we also need another test

Comment 9

[Tom Tung [:tt, :ttung]]

Comment on attachment 9010560 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Drop the review request for now. I'll revise it based on the patch in bug 1493908 and then send a review request again.

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Rebase and rename the patch

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Comment 12

[Jan Varga [:janv]]

Ok, bug 1493908 is now fixed, can you re-base/re-style the patch again ? Thanks.

Comment 13

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #12)
> Ok, bug 1493908 is now fixed, can you re-base/re-style the patch again?
> Thanks.

Sure, thanks!

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Revised the patch

Comment 15

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Comment 16

[Jan Varga [:janv]]

Attachment: original issue

Comment 17

[Jan Varga [:janv]]

Comment on attachment 9013246 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

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

Looks mostly ok, but it doesn't test the original issue reported in bug 1372116.
I attached a patch that demonstrates it, you probably need to test both situations (quota initialized and quota not initialized).

Comment 18

[Jan Varga [:janv]]

Comment on attachment 8896122 [details] [diff] [review]
Bug 1389378 - P1: Test to verify persist() do create originInfo when the temporary storage has been initialized.

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

::: dom/quota/test/unit/test_persist_trackQuota.js
@@ +17,5 @@
> +  clear(continueToNextStepSync);
> +  yield undefined;
> +
> +  info("Initialize temporary repository by initOrigin(): " + url + ".");
> +  initOrigin(getPrincipal(url), "temporary", continueToNextStepSync);

Actually, here in your old patch you do test the original issue reported in bug 1372116.

@@ +34,5 @@
> +  request = connection.open();
> +  request.callback = continueToNextStepSync;
> +  yield undefined;
> +
> +  request = connection.write(new ArrayBuffer(globalLimitKB * 1024 + 1));

Here you have just one write request, I think I like it more than writing globalLimitKB * 1024 bytes and then 1 byte

Comment 19

[Tom Tung [:tt, :ttung]]

Thanks for catching that! I somehow removed it when rewriting the test

Comment 20

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Comment 21

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Comment 22

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Revised the patch.

Comment 23

[Jan Varga [:janv]]

Attachment: suggestions (interdiff)

Comment 24

[Jan Varga [:janv]]

I think we should add a new method to nsIQuotaManagerService called initTemporaryStorage(), then we don't have to do the hack in the test to call initOrigin() for foo.com
Tom, can you do it in a separate patch ? (maybe file a new bug for it and make it block this bug)
Thanks.
I'll comment about "suggestions" later today.

Comment 25

[Jan Varga [:janv]]

Comment on attachment 9013524 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

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

::: dom/quota/test/unit/test_persist_globalLimit.js
@@ +11,5 @@
> +
> +  let request = clear();
> +  await requestFinished(request);
> +
> +  info("Persisting an origin.");

This info message doesn't belong here.

Nit: other tests omit the comman

@@ +14,5 @@
> +
> +  info("Persisting an origin.");
> +
> +  const spec = "https://persisted.example.com";
> +  const principal = getPrincipal(spec);

I prefere to declare urls, specs, stuff like this in the beginning of testSteps().
We can also just do:
const principal = getPrincipal("https://persisted.example.com");

@@ +16,5 @@
> +
> +  const spec = "https://persisted.example.com";
> +  const principal = getPrincipal(spec);
> +
> +  for (let initializeStorageBeforePersist of [true, false]) {

I think it would be more natural to test without temporary storage initialization first.

@@ +20,5 @@
> +  for (let initializeStorageBeforePersist of [true, false]) {
> +    if (initializeStorageBeforePersist) {
> +      info("Initializing an origin to initialize the temporary storage.");
> +
> +      request = initOrigin(principal, "temporary");

I guess, you use "temporary" to avoid initializing the origin that is going to be persisted.
Well, I think we should either have a dummy origin for this here and use "default" or just implement initTemporaryStorage in the QM service.

@@ +29,5 @@
> +    await requestFinished(request);
> +
> +    info("Verifying the persisted origin is bounded by global limit.");
> +
> +    let database = getSimpleDatabase(principal);

I try to keep:
"
requst = ...
await requestFinished(request)
"
in separate blocks, so I would put a blank line here

@@ +33,5 @@
> +    let database = getSimpleDatabase(principal);
> +    request = database.open("data");
> +    await requestFinished(request);
> +
> +    let buffer = getBuffer(globalLimitKB * 1024 + 1);

Nit: buffer is use only once, so you can do:
request = database.write(getBuffer(globalLimitKB * 1024 + 1));

@@ +41,5 @@
> +      ok(false, "Should have thrown");
> +    } catch (ex) {
> +      ok(true, "Should have thrown");
> +      ok(ex == NS_ERROR_FILE_NO_DEVICE_SPACE,
> +         "Persisted origin was bounded by the global limit");

I would rather have "Threw right code" here, and add another info() before the write() call.

Comment 26

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #25)

Thanks for the review!

> > +  info("Persisting an origin.");
> 
> This info message doesn't belong here.
> 
> Nit: other tests omit the comman

Will move that to the right place and remove commas for all the info message here.
 
> > +  const spec = "https://persisted.example.com";
> > +  const principal = getPrincipal(spec);
> 
> I prefere to declare urls, specs, stuff like this in the beginning of
> testSteps().
> We can also just do:
> const principal = getPrincipal("https://persisted.example.com");

I see. I put them just in front of the place calling it. Will correct that here and the future. 
 
> > +  for (let initializeStorageBeforePersist of [true, false]) {
> 
> I think it would be more natural to test without temporary storage
> initialization first.

Will do

> > +      request = initOrigin(principal, "temporary");
> 
> I guess, you use "temporary" to avoid initializing the origin that is going
> to be persisted.
> Well, I think we should either have a dummy origin for this here and use
> "default" or just implement initTemporaryStorage in the QM service.

Sure, will complete the bug 1495687 first so that it will have initTemporaryStorage() to call.

> > +    let database = getSimpleDatabase(principal);
> 
> I try to keep:
> "
> requst = ...
> await requestFinished(request)
> "
> in separate blocks, so I would put a blank line here

I see and will do.

> > +    let buffer = getBuffer(globalLimitKB * 1024 + 1);
> 
> Nit: buffer is use only once, so you can do:
> request = database.write(getBuffer(globalLimitKB * 1024 + 1));

Will do.

> @@ +41,5 @@
> > +      ok(false, "Should have thrown");
> > +    } catch (ex) {
> > +      ok(true, "Should have thrown");
> > +      ok(ex == NS_ERROR_FILE_NO_DEVICE_SPACE,
> > +         "Persisted origin was bounded by the global limit");
> 
> I would rather have "Threw right code" here, and add another info() before
> the write() call.

Got it and will correct it and add another info before calling write().

Comment 27

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Addressed comment and checked whether the patch aligns with the patch of suggestions. Will check again before sending it to review.

Comment 28

[Jan Varga [:janv]]

Comment on attachment 9013641 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

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

Nice!

::: dom/quota/test/unit/test_persist_globalLimit.js
@@ +12,5 @@
> +  info("Setting limits");
> +
> +  setGlobalLimit(globalLimitKB);
> +
> +  info("Clearing");

Nit: the clear() call here is part of "Setting limits", so this extra info("Clearing") could be removed.
It would also match the final "Resetting limits"

Comment 29

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Comment 30

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #28)
Thanks for the review!

Comment 31

[Jan Varga [:janv]]

Comment on attachment 9013677 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

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

::: dom/quota/test/unit/test_persist_globalLimit.js
@@ +1,5 @@
> +/**
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +

You can also add this comment here (just before |async function testSteps()|):
/**
 * This test is mainly to verify that persisted origins are always bounded by
 * the global limit.
 */

Comment 32

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Thanks for notifying! Addressed comment

Comment 33

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=070c8e4a29036512f1d983f6f73a7c06c93cb72b

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f495d81e201
A test for verifying persisted origins bounded by the global limit; r=janv

Comment 35

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/4f495d81e201