Closed Bug 1389378 Opened 3 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: tt, Assigned: tt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 13 obsolete files)

3.27 KB, patch
Details | Diff | Splinter Review
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.
Priority: -- → P2
Assignee: ttung → nobody
Status: ASSIGNED → NEW
Assignee: nobody → jvarga
Janv, can you take a look at this?
Flags: needinfo?(jvarga)
Yes.
Assignee: jvarga → shes050117
Flags: needinfo?(jvarga)
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
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.
Attachment #8896122 - Attachment is obsolete: true
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.
Attachment #9010560 - Flags: review?(jvarga)
Yeah, we also another test - test_persist_groupLimit.js, I already created it locally, will file a bug for that.
s/we also another test/we also need another test
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.
Attachment #9010560 - Flags: review?(jvarga)
Rebase and rename the patch
Attachment #9010560 - Attachment is obsolete: true
Ok, bug 1493908 is now fixed, can you re-base/re-style the patch again ? Thanks.
(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!
Revised the patch
Attachment #9012142 - Attachment is obsolete: true
Attachment #9012810 - Flags: review?(jvarga)
Attachment #9012810 - Flags: review?(jvarga)
Attachment #9013246 - Flags: review?(jvarga)
Attached patch original issue (obsolete) — Splinter Review
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).
Attachment #9013246 - Flags: review?(jvarga)
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
Attachment #8896122 - Flags: feedback+
Thanks for catching that! I somehow removed it when rewriting the test
Revised the patch.
Attachment #9013522 - Attachment is obsolete: true
Attachment #9013524 - Flags: review?(jvarga)
Attachment #9013305 - Attachment is obsolete: true
Attached patch suggestions (interdiff) (obsolete) — Splinter Review
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.
Depends on: 1495687
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.
Attachment #9013524 - Flags: review?(jvarga)
(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().
Addressed comment and checked whether the patch aligns with the patch of suggestions. Will check again before sending it to review.
Attachment #9013524 - Attachment is obsolete: true
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"
Attachment #9013641 - Flags: review+
(In reply to Jan Varga [:janv] from comment #28)
Thanks for the review!
Attachment #9013564 - Attachment is obsolete: true
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.
 */
Thanks for notifying! Addressed comment
Attachment #9013677 - Attachment is obsolete: true
Attachment #9014282 - Flags: review+
Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f495d81e201
A test for verifying persisted origins bounded by the global limit; r=janv
https://hg.mozilla.org/mozilla-central/rev/4f495d81e201
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.