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

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 1

[Eduard Neculaesi (:eduardn)]

Can I work on this?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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

Comment 3

[Eduard Neculaesi (:eduardn)]

Attachment: First patch

Comment 4

[Eduard Neculaesi (:eduardn)]

review ping?

Comment 5

[Eduard Neculaesi (:eduardn)]

Attachment: Second patch

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 7

[Eduard Neculaesi (:eduardn)]

Attachment: Third Patch

I renamed |urls| to |WARNING_PAGES_URLS|. Is that ok?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Eduard, I would appreciate if you could give feedback if you are willed to do the remaining change or not. Thanks!

Comment 10

[Eduard Neculaesi (:eduardn)]

Attachment: Last Patch

Yes, sorry for the delay. I uploaded a new patch and made the modifications you requested

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 12

[Eduard Neculaesi (:eduardn)]

Attachment: Last Patch (v2)

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 15

[Eduard Neculaesi (:eduardn)]

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 :)

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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!

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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!

Comment 19

[Jonathan French (:jfrench)]

Sure, I will check early next week, and/or do the back port if needed, no problems.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Thanks!

Comment 21

[Jonathan French (:jfrench)]

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.

Comment 22

[Jonathan French (:jfrench)]

Attachment: 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 23

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.