1298321 - Refactor/ add mochitest helper related to getHash

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P3)

Description

[Thomas Nguyen (:tnguyen)]

toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html
toolkit/components/url-classifier/tests/mochitest/test_classifier_changetablepref.html
toolkit/components/url-classifier/tests/mochitest/test_gethash.html
are using the same code related to getHash (addCompletionToServer, hash, addPrefixToDB), and these parts could have more usages in other tests later.
They could be moved to a helper/utils.

Comment 1

[Sébastien Blin [:sblin] [:amarok]]

Attachment: bug_1298321.patch

Comment 2

[François Marier [:francois]]

Comment on attachment 8826949 [details] [diff] [review]
bug_1298321.patch

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

It looks like head.js is missing from the patch.

::: toolkit/components/url-classifier/tests/mochitest/test_gethash.html
@@ -98,1 @@
>  function setup404() {

Did you forget to update the addCompletionToServer() calls in setup()?

Comment 3

[Sébastien Blin [:sblin] [:amarok]]

Attachment: bug_1298321.patch

Woops, indeed it's better with *head.js*

Comment 4

[François Marier [:francois]]

Comment on attachment 8827453 [details] [diff] [review]
bug_1298321.patch

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

I'll give Thomas an opportunity to also add his f+ if the patch looks good to him.

Can you submit to try before landing to ensure we didn't forget anything and that these tests still pass?

Comment 5

[Sébastien Blin [:sblin] [:amarok]]

I can't push to try (don't have the right).

Comment 6

[François Marier [:francois]]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #5)
> I can't push to try (don't have the right).

Would you like to get Commit Access Level 1?

The process is fairly straightforward: https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment 7

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8827453 [details] [diff] [review]
bug_1298321.patch

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

Lgtm.

::: toolkit/components/url-classifier/tests/mochitest/head.js
@@ +1,2 @@
> +// calculate the fullhash and send it to gethash server
> +function addCompletionToServer(list, url, mochitestUrl) {

nit: rename to serverUrl for clarity

Comment 8

[Sébastien Blin [:sblin] [:amarok]]

(In reply to François Marier [:francois] from comment #6)
> (In reply to Sébastien Blin [:sblin] [:amarok] from comment #5)
> > I can't push to try (don't have the right).
> 
> Would you like to get Commit Access Level 1?
> 
> The process is fairly straightforward:
> https://www.mozilla.org/en-US/about/governance/policies/commit/

Why not, It could be a great occasion to finally test this.


Thx for the nit, I will change this tomorrow.

Comment 9

[François Marier [:francois]]

Sébastien, are you still trying to get commit Level 1 access? If so, do you need help with this?

Comment 10

[Sébastien Blin [:sblin] [:amarok]]

Indeed I don't have the level 1 access. 

I know there is a wiki page which describes the try-to-push process, so I think I can do this (but i need the level 1 first)

Comment 11

[François Marier [:francois]]

Thomas, would you have time to take this patch to try to see if it's ready to be merged?

Comment 12

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0018f5ac61c5c519787410622d64815876b70c86

Comment 13

[François Marier [:francois]]

Try looks good. I think this patch is ready to go.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71897bf73d33
Refactor tests related to getHash. r=francois

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71897bf73d33