Tom Tung [:tt, :ttung] Assignee
	Description • 7 years ago

We decide to move the test in bug 1298329 to here.

	Tom Tung [:tt, :ttung] Assignee
	Comment 5 • 6 years ago

Comment on attachment 9004482 [details] [diff] [review]
Implement a test to verify the behavior of eviction.

Hi Jan,

I add this test to try to verify the persisted origin won't be evicted, but general origin will.

Could you help me to review that? Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 6 • 6 years ago

Comment on attachment 9004482 [details] [diff] [review]
Implement a test to verify the behavior of eviction.

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

	Tom Tung [:tt, :ttung] Assignee
	Comment 7 • 6 years ago

Revise the patch:
- Using requestFinished
- Rebase on the top of patch in bug 1493908

	Tom Tung [:tt, :ttung] Assignee
	Comment 8 • 6 years ago

Comment on attachment 9011830 [details] [diff] [review]
Implement a test to verify the behavior of eviction.

Jan, I rebase this patch based on your patch in Bug 1493908. This patch tests two eviction mechanism in QM.

The function testEviction() store the given origin at first to make it become the oldest origin. Then, store four other origins to full the disk so that the eviction will be triggered by storing one byte more in another origin. At the end of this function, it checks the directory of oldest/given origin to see whether it was evicted. This test expected that if the oldest origin is not a persisted origin, then it should be evicted. If it's, then it shouldn't be evicted.

	Tom Tung [:tt, :ttung] Assignee
	Comment 9 • 6 years ago

Comment on attachment 9011830 [details] [diff] [review]
Implement a test to verify the behavior of eviction.

Based on Bug 1493908 comment 6, it seems that this patch might need to use async/await as well.

	Tom Tung [:tt, :ttung] Assignee
	Comment 10 • 6 years ago

Using async/await

	Tom Tung [:tt, :ttung] Assignee
	Comment 13 • 6 years ago

Jan, I think this patch is ready to be reviewed. Could you review that? Thanks!

This patch adds a test to verify a temporary storage is evicted when the usage is about to exceed the limit and a persisted storage isn't in the same condition.

	Jan Varga [:janv]
	Comment 14 • 6 years ago

Working on it, I'm almost done.

	Jan Varga [:janv]
	Comment 15 • 6 years ago

I think the basic idea is very simple (if the oldest origin is persisted then it shouldn't be evicted) so the test can look much simpler too.
I'll send review comments later.

	Jan Varga [:janv]
	Comment 16 • 6 years ago

Comment on attachment 9012808 [details] [diff] [review]
Implement a test to verify the behavior of eviction.

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

::: dom/quota/test/unit/test_eviction.js
@@ +6,5 @@
> +const groupLimitKB = 10 * 1024;
> +// The group limit is calculated as 20% of the global limit.
> +const globalLimitKB = groupLimitKB * 5;
> +
> +async function openSimpleDBWithSize(origin, size) {

The name of this function indicates that a database will be opened and returned in that state. However, it does something else. It opens a database, writes data, closes the database and returns nothing. I would rather name it fillOrigin().

@@ +11,5 @@
> +  let database = getSimpleDatabase(getPrincipal(origin));
> +  let request = database.open("dummyName");
> +  await requestFinished(request);
> +
> +  let buffer = getRandomBuffer(size);

getRandomBuffer() is very slow for big sizes, I don't think random data is important here. getBuffer() should be enough.

@@ +26,5 @@
> +}
> +
> +async function testEviction(originToTest, dirToTest, evictionExpected) {
> +  info("Step 1: Storing a origin with full usage");
> +  await openSimpleDBWithSize(originToTest, groupLimitKB * 1024);

This 1st origin can be filled in one loop together with those 4 origins below.

@@ +33,5 @@
> +    "https://group1.example1.com",
> +    "https://group2.example2.com",
> +    "https://group3.example3.com",
> +    "https://group4.example4.com"
> +  ];

There's obviously a simple pattern, so we can dynamically create these urls in a loop.

@@ +41,5 @@
> +  for (let origin of originsToFullDisk) {
> +    await openSimpleDBWithSize(origin, groupLimitKB * 1024);
> +  }
> +
> +  info("Step 3: Attempting to trigger eviction");

"Attempting" indicates that it may or may not happen. The thing is that eviction will be triggered no matter what.
If the oldest origin is not persisted then the first origin will be evicted.
If the oldest origin is persisted then the second origin will be evicted.

@@ +42,5 @@
> +    await openSimpleDBWithSize(origin, groupLimitKB * 1024);
> +  }
> +
> +  info("Step 3: Attempting to trigger eviction");
> +  const originMaybeTriggerEviction = "http://url.to.trigger.com";

dtto

@@ +43,5 @@
> +  }
> +
> +  info("Step 3: Attempting to trigger eviction");
> +  const originMaybeTriggerEviction = "http://url.to.trigger.com";
> +  await openSimpleDBWithSize(originMaybeTriggerEviction, 1);

This 6th origin can be filled in one loop together with those 4 origins above.

@@ +49,5 @@
> +  info("Step 4: Verifying if the oldest origin was evicted or not");
> +  let oldestOriginDir = getRelativeFile(dirToTest);
> +  ok(!oldestOriginDir.exists() == evictionExpected,
> +     evictionExpected ? "The origin " + originToTest + " was evicted."
> +                      : "The origin " + originToTest + " was not evicted.");

It would be safer to check all origin directories.

@@ +59,5 @@
> +    {
> +      origin: "http://best-effort.example.com",
> +      directory: "storage/default/http+++best-effort.example.com",
> +      evictionExpected: true,
> +      persisted: false,

persisted is redundant since it can be expressed as !evictionExpected

@@ +69,5 @@
> +      persisted: true,
> +    }
> +  ];
> +
> +  setGlobalLimit(globalLimitKB);

setGlobalLimit and resetGlobalLimit should be always paired with clear() or reset(), otherwise it won't take any effect.

::: dom/quota/test/unit/xpcshell.ini
@@ +22,5 @@
>  
>  [test_basics.js]
>  [test_bad_origin_directory.js]
>  [test_defaultStorageUpgrade.js]
> +[test_eviction.js]

I don't think this a generic test for eviction. I would name it test_persist_eviction.js

	Tom Tung [:tt, :ttung] Assignee
	Comment 17 • 6 years ago

(In reply to Jan Varga [:janv] from comment #16)
Thanks for the review! I still have a lot of things to learn.

Some comments can be applied on patches in bug 1389378 and bug 1389380. I will drop the requests of them and revise them again as well.

I wanted to make this test generic so that it could be extended easily, but it made the test lose its efficacy. Also, that's no any reason to do that now, I could make them generic if it's needed in the future.

> > +async function openSimpleDBWithSize(origin, size) {
> 
> The name of this function indicates that a database will be opened and
> returned in that state. However, it does something else. It opens a
> database, writes data, closes the database and returns nothing. I would
> rather name it fillOrigin().

I see. OpenXXXDatabase() does imply to return the handle of a database.

> @@ +11,5 @@
> > +  let buffer = getRandomBuffer(size);
> 
> getRandomBuffer() is very slow for big sizes, I don't think random data is
> important here. getBuffer() should be enough.

I see.

I saw test_simpledb.js use that, but it's because it compared the content of databases after that. I also checked the functions in "unit/head.js", but I neglected to check functions in "head-shared.js". I should search all the function name and compare the relatives before using them.

> > +  await openSimpleDBWithSize(originToTest, groupLimitKB * 1024);
> 
> This 1st origin can be filled in one loop together with those 4 origins
> below.

I considered about that. I was worry that it's hard to point out the certain order of origin without a magic number, but it seems that it may not be as bad as I thought.

> > +    "https://group4.example4.com"
> > +  ];
> 
> There's obviously a simple pattern, so we can dynamically create these urls
> in a loop.

That's a good point, I should have noticed that.
> +  info("Step 3: Attempting to trigger eviction");
> 
> "Attempting" indicates that it may or may not happen. The thing is that
> eviction will be triggered no matter what.
> If the oldest origin is not persisted then the first origin will be evicted.
> If the oldest origin is persisted then the second origin will be evicted.

Indeed, I was trying to focus on the testing origin. I guess I should have written: "Attempting to evict XXX origin".

> @@ +42,5 @@
> > +    await openSimpleDBWithSize(origin, groupLimitKB * 1024);
> > +  }
> > +
> > +  info("Step 3: Attempting to trigger eviction");
> > +  const originMaybeTriggerEviction = "http://url.to.trigger.com";
> 
> dtto
> 
> @@ +43,5 @@
> > +  }
> > +
> > +  info("Step 3: Attempting to trigger eviction");
> > +  const originMaybeTriggerEviction = "http://url.to.trigger.com";
> > +  await openSimpleDBWithSize(originMaybeTriggerEviction, 1);
> 
> This 6th origin can be filled in one loop together with those 4 origins
> above.

I was trying to reduce the size of buffer written into the disk. 

> @@ +49,5 @@
> > +  info("Step 4: Verifying if the oldest origin was evicted or not");
> > +  let oldestOriginDir = getRelativeFile(dirToTest);
> > +  ok(!oldestOriginDir.exists() == evictionExpected,
> > +     evictionExpected ? "The origin " + originToTest + " was evicted."
> > +                      : "The origin " + originToTest + " was not evicted.");
> 
> It would be safer to check all origin directories.

That's true.

> @@ +59,5 @@
> > +      persisted: false,
> 
> persisted is redundant since it can be expressed as !evictionExpected

That's correct.

> @@ +69,5 @@
> > +      persisted: true,
> > +    }
> > +  ];
> > +
> > +  setGlobalLimit(globalLimitKB);
> 
> setGlobalLimit and resetGlobalLimit should be always paired with clear() or
> reset(), otherwise it won't take any effect.

I didn't call rest() at the end of this test (I assumed every test call clear() at the beginning of themselves, but I found it's problematic to assume that), but here clear() was called after that in the for-loop (or it's because it might confuse others since it's not right after that?).

> ::: dom/quota/test/unit/xpcshell.ini
> @@ +22,5 @@
> >  
> >  [test_basics.js]
> >  [test_bad_origin_directory.js]
> >  [test_defaultStorageUpgrade.js]
> > +[test_eviction.js]
> 
> I don't think this a generic test for eviction. I would name it
> test_persist_eviction.js

Sure

	Tom Tung [:tt, :ttung] Assignee
	Comment 18 • 6 years ago

Addressed comments, merged the inter-diff patch and modified commit message

	Jan Varga [:janv]
	Comment 19 • 6 years ago

Comment on attachment 9013231 [details] [diff] [review]
Bug 1389390 - Add a test to verify a persisted origin won't be evicted; r=janv

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

Looks good, thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 20 • 6 years ago

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=674550da1cbbeb7d52c17ef46add4a062cb6810a

	Pulsebot
	Comment 21 • 6 years ago

Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed04fa44d07
Add a test to verify a persisted origin won't be evicted; r=janv

	Cristian Brindusan [:cbrindusan]
	Comment 22 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3ed04fa44d07