1037555 - safebrowsing listmanager needs a unittest

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

Since this module ties together DBService and streamupdater, it seriously needs a unittest.

Comment 1

[François Marier [:francois]]

Henry, I'm going to assign this to you since that sounds like a good first step in refactoring the list manager.

Comment 2

[Henry Chang [:hchang]]

nsIUrlListManager has the following interfaces:

getGethashUrl
registerTable
enableUpdate
disableUpdate
maybeToggleUpdateChecking
safeLookup

Since the core of listmanager is to maintain |tablesData| and to schedule its update, what I'd like to test at the beginning is registerTable/enableUpdate/disableUpdate/getGethashUrl. Given that there's no interface to obtain the internal state (except for getGethashUrl), I'd like to indirectly do the check from the server side: 

"Check if the expected request body is sent."

Some issues are coming to my mind:

1) Timing. It may be solved by tackling Date.now or change nextupdatetime to 1 to force an update.
2) nsIUrlListManager is a service so that some tables/urls might have been registered. We have to avoid the tests being interfered by the registered tables.
3) If we register testing tables/urls, would it break DBservice or any other components? I need to investigate this.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#57

Comment 3

[Henry Chang [:hchang]]

Hi Francois,

Could I have your comments for the very first testing plan for listmanager? I haven't got started to implement it and would like to know if it's feasible and worth doing. Thanks :)

Comment 4

[Henry Chang [:hchang]]

Attachment: Initial listmanager test case.

Comment 5

[Henry Chang [:hchang]]

Attached the experimental (but totally works and is testing something!) test case. 

To address my worries in comment 2:

1) The nextupdatetime trick rocks!
2) SafeBrowsing.jsm seems not running in xpcshell test.
3) Totally fine as far as I can tell.

Comment 6

[Henry Chang [:hchang]]

What I test initially in the test case is:

1) Register 3 tables for the same update URL.
2) Enable two of them, force an update and see if the expected request is sent.
3) Enable one of them, force an update and see if the expected request is sent.

No meaningful response is replied to the client so some cases are not covered like:

"We don't verify if listmanager would collect existing chunks from DBService and send to the server."

I believe this can be done easily.

Also, what else we can test are:

1) Register tables for multiple update URLs (only one update URL in the test case for now)
2) Verify if the update will be triggered on schedule (right now we just force to update)
3) getGethashUrl (should be trivial)
4) backoff (no good idea so far)
5) updateSuccess/updateError/downloadError

Comment 7

[Henry Chang [:hchang]]

Attachment: Updated listmanager test case

Comment 8

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #6)
> What I test initially in the test case is:
> 
> 1) Register 3 tables for the same update URL.
> 2) Enable two of them, force an update and see if the expected request is
> sent.
> 3) Enable one of them, force an update and see if the expected request is
> sent.
> 
> No meaningful response is replied to the client so some cases are not
> covered like:
> 
> "We don't verify if listmanager would collect existing chunks from DBService
> and send to the server."
> 
> I believe this can be done easily.
> 
> Also, what else we can test are:
> 
> 1) Register tables for multiple update URLs (only one update URL in the test
> case for now)
> 2) Verify if the update will be triggered on schedule (right now we just
> force to update)
> 3) getGethashUrl (should be trivial)
> 4) backoff (no good idea so far)
> 5) updateSuccess/updateError/downloadError

Refined the test case. What's new:

1) Multiple update URLs (by using different port)
2) Server would respond add/subchunk control so that we can test the request associated with
   existing chunks.
3) nsIUrlListManager.getGethashUrl

I believe the test case is already in shape so request a review.

Comment 9

[François Marier [:francois]]

Comment on attachment 8751650 [details] [diff] [review]
Updated listmanager test case

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

That looks good to me, but please rename the test lists so that they follow the "name-type-format" pattern we see elsewhere.

::: toolkit/components/url-classifier/tests/unit/test_listmanager.js
@@ +7,5 @@
> +// These tables share the same updateURL.
> +const TEST_TABLE_DATA_LIST = [
> +  // 0:
> +  {
> +    tableName: "test-listmanager-0-digest256",

Interesting, I'm surprised this list name works :)

I would suggest that we stick to the usual 3-part format though "test-listmanager0-digest256" to avoid any problems.

@@ +209,5 @@
> +
> +  run_next_test();
> +}
> +
> +// The nextupdatetime might be set as string in listmanager::updateSuccess_,

Do you think that's something we should fix (in a follow-up bug)? It seems like a mistake that this pref would be stored as a string.

@@ +231,5 @@
> +  TEST_TABLE_DATA_LIST.forEach(t => gListManager.disableUpdate(t.tableName));
> +  gListManager.disableUpdate(TEST_TABLE_DATA_ANOTHER.tableName);
> +}
> +
> +// Since there's no public interface on listmanager to know the update success,

That reminds me of https://bugzilla.mozilla.org/show_bug.cgi?id=457790 which I wasn't sure about.

Comment 10

[Henry Chang [:hchang]]

Thanks François!

(In reply to François Marier [:francois] from comment #9)
> Comment on attachment 8751650 [details] [diff] [review]
> Updated listmanager test case
> 
> Review of attachment 8751650 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: toolkit/components/url-classifier/tests/unit/test_listmanager.js
> @@ +7,5 @@
> > +// These tables share the same updateURL.
> > +const TEST_TABLE_DATA_LIST = [
> > +  // 0:
> > +  {
> > +    tableName: "test-listmanager-0-digest256",
> 
> Interesting, I'm surprised this list name works :)
> 
> I would suggest that we stick to the usual 3-part format though
> "test-listmanager0-digest256" to avoid any problems.
> 

From what I know, only the first part and the last part matters.

Given that there has been no problem for the current format, 
do you think the test could help us prevent from making additional 
assumption on the format? (since relying on the format doesn't seem to
be a good practice to me :( )

> @@ +209,5 @@
> > +
> > +  run_next_test();
> > +}
> > +
> > +// The nextupdatetime might be set as string in listmanager::updateSuccess_,
> 
> Do you think that's something we should fix (in a follow-up bug)? It seems
> like a mistake that this pref would be stored as a string.
> 

It seems |nextupdatetime| is always stored as a string and there's an implicit
conversion here [1]. It's fine for the current use but once we accidentally add
the string to a number ... (e.g. "123" + 5)

We should definitely fix this mistake! Will file a bug for this :)

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#239

Comment 11

[Henry Chang [:hchang]]

Attachment: Updated listmanager test case (r+'ed)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0048d886070

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e0048d886070