Closed Bug 1037555 Opened 6 years ago Closed 4 years ago

safebrowsing listmanager needs a unittest

Categories

(Toolkit :: Safe Browsing, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: mmc, Assigned: hchang)

References

Details

Attachments

(2 files, 1 obsolete file)

Since this module ties together DBService and streamupdater, it seriously needs a unittest.
Component: DOM: Security → Safe Browsing
Product: Core → Toolkit
Henry, I'm going to assign this to you since that sounds like a good first step in refactoring the list manager.
Assignee: nobody → hchang
Blocks: 1264885
Priority: -- → P2
Status: NEW → ASSIGNED
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
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 :)
Flags: needinfo?(francois)
Attached patch Initial listmanager test case. (obsolete) — Splinter Review
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.
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
Attachment #8751296 - Attachment is obsolete: true
(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.
Flags: needinfo?(francois)
Attachment #8751650 - Flags: review?(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.
Attachment #8751650 - Flags: review?(francois) → review+
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
Flags: needinfo?(francois)
https://hg.mozilla.org/mozilla-central/rev/e0048d886070
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Flags: needinfo?(francois)
You need to log in before you can comment on or make changes to this bug.