656288 - Have only one test function in testPasswordSavedandDeleted.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

Split testPasswordSavedandDeleted.js into two modules: 

*testPasswordSaved.js https://litmus.mozilla.org/show_test.cgi?id=8172
*testPasswordDeleted.js https://litmus.mozilla.org/show_test.cgi?id=8173

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Disabled https://litmus.mozilla.org/show_test.cgi?id=8172 in Litmus

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Modified https://litmus.mozilla.org/show_test.cgi?id=8173. Merged information from https://litmus.mozilla.org/show_test.cgi?id=8172

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Vlad, both referenced Litmus tests are from the 3.6 branch on Litmus. Please only disable the test from the Aurora branch, but no older branch. Also see the following link how to correctly disable a Litmus test. We should probably spread this around so everyone knows about it.

https://wiki.mozilla.org/QA/Execution/Litmus#Disabling_of_Litmus_Tests

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

Disabled https://litmus.mozilla.org/show_test.cgi?id=15637 on Aurora branch and modified https://litmus.mozilla.org/show_test.cgi?id=15638

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

Attachment: Patch v1.0

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

Attachment: Patch v1.1

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

Patch v1.1 with some minor changes due to Comment 7 in bug 599148

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

Patch v1.1 with some minor changes due to bug 599148 Comment 7

Comment 10

[u279076]

Comment on attachment 531898 [details] [diff] [review]
Patch v1.1

>+// Test if Password is saved and deleted
>+var testSaveAndDeletePassword = function() {

Nit: Please make this a named function using the format "function testFunction() {"

Comment 11

[Maniac Vlad Florin (:vladmaniac)]

Attachment: Patch v1.2

Comment 12

[u279076]

Comment on attachment 532658 [details] [diff] [review]
Patch v1.2

>+// Test if Password is saved and deleted
>+function testSaveAndDeletePassword() {

Please use /* */ style commenting to explain the function

Other than that, this patch looks fine. r+ given that change. Over to Henrik for final review.

Comment 13

[Maniac Vlad Florin (:vladmaniac)]

Attachment: Patch v1.3

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

Thanks for the review+ Anthony. Nit fixed in patch v1.3 which made patch v1.2 obsolete. r = hskupin

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 533217 [details] [diff] [review]
Patch v1.3

Vlad, please combine the changes into one single patch.

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

Attachment: Patch v1.4

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 533288 [details] [diff] [review]
Patch v1.4

>- * Portions created by the Initial Developer are Copyright (C) 2009
>+ * Portions created by the Initial Developer are Copyright (C) 2011

Please don't change the date while updating an existent test. That's the original date when the test has been added.

> var teardownModule = function() {

Change it to a named function. Same applies to setupModule. Seems like you simply forgot it.

>+function testSaveAndDeletePassword() {
>+
>   // Go to the sample login page and log-in with inputted fields

nit: no need for an empty line here.

>   var logInButton = new elementslib.ID(controller.tabs.activeTab, "LogIn");
>   controller.click(logInButton);
>-  controller.sleep(500);
>+  controller.waitForPageLoad();

Good catch. Thanks!

>-var prefDialogCallback = function(controller) {
>+  var prefDialogCallback = function(controller) {

Correct the indentation here and also use a named definition.

With those changes r=me.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh, and please use hg export too, so that the patch includes your username and the commit message. Thanks.

Comment 19

[Maniac Vlad Florin (:vladmaniac)]

(In reply to comment #10)
> Comment on attachment 531898 [details] [diff] [review] [review]
> Patch v1.1
> 
> >+// Test if Password is saved and deleted
> >+var testSaveAndDeletePassword = function() {
> 
> Nit: Please make this a named function using the format "function
> testFunction() {"

Sorry for not changing those, still I haven't missed them. In the review is requested a change only for this specific function. Now that is clear, I will make the changes

Comment 20

[Maniac Vlad Florin (:vladmaniac)]

Attachment: Patch File v1.5

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 533601 [details] [diff] [review]
Patch File v1.5

>- * Portions created by the Initial Developer are Copyright (C) 2011
>+ * Portions created by the Initial Developer are Copyright (C) 2009

This patch doesn't apply cleanly and given this lines it seems like to be an interim diff again against your last patch.

Comment 22

[Maniac Vlad Florin (:vladmaniac)]

Attachment: Patch v1.6

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 533928 [details] [diff] [review]
Patch v1.6

Ideally we could also have been get rid of the TIMEOUT constant but it's not a blocker for r+ right now. Lets move on and get it removed later. Thanks Vlad.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/b7e34cfceb02 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/36f1e5dd58d0 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/c211c062c5b5 (beta)

I will leave it open until the Litmus tests have been updated.

Comment 25

[Maniac Vlad Florin (:vladmaniac)]

Updated https://litmus.mozilla.org/show_test.cgi?id=15638