Move test URLs to globally defined constants for testSecurity/testSafeBrowsing*.js tests

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vladmaniac, Assigned: jfrench)

Tracking

unspecified

Firefox Tracking Flags

(firefox19 fixed, firefox20 fixed, firefox-esr10 wontfix, firefox-esr17 fixed)

Details

(Whiteboard: [mentor=whimboo][lang=js][good first bug], URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
We just want to modify this code snippet 
>  var testWarningPages = function() {
> +  var urls = ['http://' + DOMAIN_NAME + '/firefox/its-a-trap.html',
> +              'http://' + DOMAIN_NAME + '/firefox/its-an-attack.html'];

and move this in the global scope as constants

The file to be changed is located in mozmill-tests/tests/functional/testSecurity/testSafeBrowsingWarningPages.js under hg.mozilla.org/qa section

Link to the test file to be updated: 
http://hg.mozilla.org/qa/mozmill-tests/file/56c01340f467/tests/functional/testSecurity/testSafeBrowsingWarningPages.js
(Reporter)

Updated

5 years ago
Summary: Have the urls as global constants in tests/functional/testSecurity/testSafeBrowsingWarningPages.js → Move test URLs to globally defined constants for testSecurity/testSafeBrowsingWarningPages.js
Whiteboard: [mentor=whimboo][lang=js][good first bug]
Assignee: nobody → marioalv.mozilla
Assignee: marioalv.mozilla → nobody
Can I work on this?
Eduard, you are welcome to do so. If you have questions feel free to ask or better join us on IRC in the #automation channel.

General information about Mozmill tests you can find here:
https://developer.mozilla.org/en/Mozmill_Tests
Assignee: nobody → eduardnem
Created attachment 674024 [details] [diff] [review]
First patch
Attachment #674024 - Flags: review?(hskupin)
review ping?
Created attachment 674662 [details] [diff] [review]
Second patch
Attachment #674024 - Attachment is obsolete: true
Attachment #674024 - Flags: review?(hskupin)
Attachment #674662 - Flags: review?(hskupin)
Comment on attachment 674662 [details] [diff] [review]
Second patch

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

Thanks Eduard for the patch. Sorry that it has been taken a while until I was able to look at it. But here comes my review as requested by yourself yesterday.

::: tests/functional/testSecurity/testSafeBrowsingWarningPages.js
@@ +12,5 @@
>  const gTimeout = 5000;
>  
>  const DOMAIN_NAME = "www.mozilla.org";
> +const urls = ['http://' + DOMAIN_NAME + '/firefox/its-a-trap.html',
> +            'http://' + DOMAIN_NAME + '/firefox/its-an-attack.html'];

For constants we usually use capital letters. So urls has to follow here and further updates are needed in the test methods.

Also ensure you have the right indentation for the second line, so both start at the same column.
Attachment #674662 - Flags: review?(hskupin) → review-
Created attachment 675329 [details] [diff] [review]
Third Patch

I renamed |urls| to |WARNING_PAGES_URLS|. Is that ok?
Attachment #674662 - Attachment is obsolete: true
Attachment #675329 - Flags: review?(hskupin)
Comment on attachment 675329 [details] [diff] [review]
Third Patch

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

Looks fine to me. Eduard, would you mind to also update the testSafeBrowsingNotificationBar.js file with the same changes? I would appreciate it. Also sorry for the late reply but I was away for a while.
Attachment #675329 - Flags: review?(hskupin) → review+
Summary: Move test URLs to globally defined constants for testSecurity/testSafeBrowsingWarningPages.js → Move test URLs to globally defined constants for testSecurity/testSafeBrowsing*.js tests
Eduard, I would appreciate if you could give feedback if you are willed to do the remaining change or not. Thanks!
Created attachment 687918 [details] [diff] [review]
Last Patch

Yes, sorry for the delay. I uploaded a new patch and made the modifications you requested
Attachment #675329 - Attachment is obsolete: true
Attachment #687918 - Flags: review?(hskupin)
Comment on attachment 687918 [details] [diff] [review]
Last Patch

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

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +9,5 @@
>  
>  const gDelay = 0;
>  const gTimeout = 5000;
>  
> +const WARNING_PAGES_URLS = ['http://' + DOMAIN_NAME + '/firefox/its-a-trap.html',

You missed to define DOMAIN_NAME as for the other test. Please make sure you run the test at least ones to verify that it doesn't fail.
Attachment #687918 - Flags: review?(hskupin) → review-
Created attachment 689596 [details] [diff] [review]
Last Patch (v2)
Attachment #687918 - Attachment is obsolete: true
Attachment #689596 - Flags: review?(hskupin)
Comment on attachment 689596 [details] [diff] [review]
Last Patch (v2)

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

Looks fine now Eduard! Thank you for the work. If you are further interested to help out on other bugs please let me know or query for other mentored bugs in Bugzilla.
Attachment #689596 - Flags: review?(hskupin) → review+
http://hg.mozilla.org/qa/mozmill-tests/rev/b303b1849c66 (default)

I kinda would like to see it fixed across branches. So lets watch out for regressions. If everything is fine we can get this patch backported. Leaving bug open until it is fixed on all branches.
Status: NEW → ASSIGNED
status-firefox-esr10: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → fixed
status-firefox-esr17: --- → affected
Great, Thanks!

I'm interested to help out on other bugs. I'll search for other mentored bugs but feel free to assign me some :)
Eduard, before I can close this bug as fixed we want to backport your patch to older branches. Would you mind to check if it applies correctly onto those? A test with the appropriate version of Firefox would be required too. Thanks!
Eduard, I want to ask if you are still interested to get this patch backported. If not, someone else would have to cover this. Thanks.
status-firefox-esr10: affected → wontfix
Jonathan, would you mind to check for the backports on that bug and if they apply on release and esr17? I would kinda like to see this bug finished. Thanks!
status-firefox17: affected → ---
status-firefox18: affected → ---
(Assignee)

Comment 19

5 years ago
Sure, I will check early next week, and/or do the back port if needed, no problems.
Thanks!
Assignee: eduardnem → tojonmz
(Assignee)

Comment 21

5 years ago
Eduard's patch "Last Patch (v2)" can be applied to Release and Esr17, without any resolves required. I've tested and confirmed that both of these branches run and pass with this patch applied.

Tested with Release 19.0 20130215130331. Tests pass as expected.
Tested with 17.0.3esrpre 20130224034501. Tests pass as expected.

I did notice that these two tests, also are among the tests whose function() blocks have been cleaned up in bug 842058 (in the default branch only). It's likely outside the scope of what you want to address with this specific bug, but in case it is desired I will provide an alternate patch which also incorporates fixes to those bad function blocks for these two tests. A patch you can either use or discard.
(Assignee)

Comment 22

5 years ago
Created attachment 717703 [details] [diff] [review]
move test urls to constants (release, esr17)

Optional patch "move test urls to constants (release, esr17)" for those respective branches. Two files modified. This patch is a superset of "Last Patch (v2)", it also contains fixes to two function() blocks, which were addressed in the equivalent files in default.

Tested with Release 19.0 20130215130331. Tests pass as expected.
Tested with 17.0.3esrpre 20130224034501. Tests pass as expected.

Intentionally not setting review flag, as this patch is at Henrik's discretion.
Comment on attachment 717703 [details] [diff] [review]
move test urls to constants (release, esr17)

Lets keep the original patch so we are in sync with the higher branches on that.
Attachment #717703 - Attachment is obsolete: true
Landed on other branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/cf93e30da264 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/f48f4494a098 (esr17)

Thank you Jonathan for checking that. Means we are done now.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox19: affected → fixed
status-firefox-esr17: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.