1274223 - Test if nsUrlClassifierHashCompleter.js sends expected completion request.

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Henry Chang [:hchang]]

It's currently unable to test since the prefixes would be shuffled before being assembled to one single HTTP request. What we can do is either of the following:

1) Add a preference and modify nsUrlClassifierHashCompleter.js to avoid shuffling prefixes while running the test case.

2) Do not byte-wise compare the request. Compare if all prefixes are present in the request. (split the request according to the prefix length then compare ...)

3) Only verify the request when there is only one prefix in the request.

I personally prefer (1).

Comment 1

[Henry Chang [:hchang]]

Attachment: WIP patch

Comment 2

[Henry Chang [:hchang]]

If we do (1) (mentioned in the description), the test is as simple as attachment 8754305 [details] [diff] [review]

Comment 3

[Henry Chang [:hchang]]

Hi Francois, May I have your feedback on this bug? Thanks :)

Comment 4

[François Marier [:francois]]

(In reply to Henry Chang [:henry] from comment #0)
> It's currently unable to test since the prefixes would be shuffled before
> being assembled to one single HTTP request. What we can do is either of the
> following:
> 
> 1) Add a preference and modify nsUrlClassifierHashCompleter.js to avoid
> shuffling prefixes while running the test case.

I'm not sure I understand what you mean by the shuffling of the prefixes. Do you mean that if you request a completion for more than one prefix, they are reordered randomly?

If so, one thing we could do is sort them. That way the order would be deterministic and we could rely on it in our tests.

I'm a little bit wary of adding a pref just for our tests because then we're not actually testing the real configuration. Sometime that's inevitable, but if we can avoid it that would be great.

> 2) Do not byte-wise compare the request. Compare if all prefixes are present
> in the request. (split the request according to the prefix length then
> compare ...)

Whilte that's more work, it sounds like a good approach.

> 3) Only verify the request when there is only one prefix in the request.

That option wouldn't work because we always send more than one prefix in completion requests (we add 3 "noise" entries by default).

Comment 5

[Henry Chang [:hchang]]

Attachment: Patch v1

Comment 6

[François Marier [:francois]]

Comment on attachment 8755379 [details] [diff] [review]
Patch v1

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

::: toolkit/components/url-classifier/tests/unit/test_hashcompleter.js
@@ +141,5 @@
> +
> +  let partialLength = parseInt(tokens[1]);
> +  let payloadLength = parseInt(tokens[2]);
> +
> +  let payloadStart = tokens[1].length + // partial length

These comments are very useful in making this part easy to understand. Thanks!

Comment 7

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e2c782d3549

Comment 8

[Mark Banner (:standard8)]

This landed with the wrong bug number (it had landed with bug 1274226):

https://hg.mozilla.org/integration/fx-team/rev/f03b60a446f1
https://hg.mozilla.org/mozilla-central/rev/f03b60a446f1