Refactor/ add mochitest helper related to getHash

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Safe Browsing
P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: tnguyen, Assigned: sblin)

Tracking

({good-first-bug})

unspecified
mozilla55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: #sbv4-m6)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Priority: -- → P5
Priority: P5 → P3
Blocks: 1276826
Blocks: 1329808
No longer blocks: 1276826
(Assignee)

Comment 1

11 months ago
Created attachment 8826949 [details] [diff] [review]
bug_1298321.patch
Attachment #8826949 - Flags: review?(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()?
Attachment #8826949 - Flags: review?(francois) → review-
(Assignee)

Comment 3

11 months ago
Created attachment 8827453 [details] [diff] [review]
bug_1298321.patch

Woops, indeed it's better with *head.js*
Attachment #8826949 - Attachment is obsolete: true
Attachment #8827453 - Flags: review?(francois)
Assignee: nobody → amarok
Status: NEW → ASSIGNED
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?
Attachment #8827453 - Flags: review?(francois)
Attachment #8827453 - Flags: review+
Attachment #8827453 - Flags: feedback?(tnguyen)
(Assignee)

Comment 5

11 months ago
I can't push to try (don't have the right).
(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/
(Reporter)

Updated

11 months ago
Keywords: good-first-bug
(Reporter)

Comment 7

11 months ago
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
Attachment #8827453 - Flags: feedback?(tnguyen) → feedback+
(Assignee)

Comment 8

11 months ago
(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.
Sébastien, are you still trying to get commit Level 1 access? If so, do you need help with this?
Flags: needinfo?(amarok)
(Assignee)

Comment 10

9 months ago
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)
Flags: needinfo?(amarok)
Thomas, would you have time to take this patch to try to see if it's ready to be merged?
Flags: needinfo?(tnguyen)
Whiteboard: #sbv4-m6
(Reporter)

Comment 12

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0018f5ac61c5c519787410622d64815876b70c86
(Reporter)

Updated

9 months ago
Flags: needinfo?(tnguyen)
Try looks good. I think this patch is ready to go.
Keywords: checkin-needed

Comment 14

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71897bf73d33
Refactor tests related to getHash. r=francois
Keywords: checkin-needed

Comment 15

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71897bf73d33
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.