Refactor tests which use getNotification() method

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jfrench, Assigned: jfrench)

Tracking

unspecified

Firefox Tracking Flags

(firefox29 fixed)

Details

(Whiteboard: [mentor= andreea.matei][lang=js][good first bug], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Just entering a marker bug, begat from bug 758187, for the potential refactoring of tests which use getNotification() and the related method changes in that bug.

At present, we think changes may involve these tests

testPreferences/testPasswordSavedAndDeleted
testPasswordManager/testPasswordNotificationBar
restartTests/testPreferences_masterPassword/test1

I searched for this bug beforehand, so hopefully I'm not entering a duplicate bug. :) Apologies if so.
Severity: minor → normal
Whiteboard: [mentor= andreea.matei][lang=js][good first bug]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → tojonmz
(Assignee)

Updated

6 years ago
Depends on: 758187
(Assignee)

Comment 1

6 years ago
I updated the tests in a local branch last night, they all appear to work fine with the new method provided in lib/toolbars.js in patchv6 provided in bug 758187. 

I will hold off on uploading any patches until that bug is landed on default.

I also flipped the boolean intentionally to true in the tests (notification popup expected open) for a test run, just to confirm that the method then identifies a simulated failure condition.

Tested with Default 22.0a1 20130329030904. Tests pass as expected.
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
Created attachment 755136 [details] [diff] [review]
updates to waitForNotification (default) rev1.0

Patch "updates for waitForNotification (default) rev1.0" for Default branch. Three files modified.

The work represents updates to tests, in harmony with the latest patch(v6.1) for waitForNotification() used in bug 758187.

It's been quite awhile since I looked at this patch locally, please give it a careful look, I could well have misinterpreted the change. 

It seemed timely to provide an initial patch for review, as it seems we are getting close to landing the change to toolbars.js for testGeolocation. Maybe it would make sense for this patch to be merged with that patch before landing, I'm not sure. Since this patch requires that one's toolbars.js.

Tested with Default Nightly 24.0a1 20130528030942 using Andreea's latest lib/toolbars.js from patch rev6.1. All tests pass as expected.
Attachment #755136 - Flags: review?(andreea.matei)
Comment on attachment 755136 [details] [diff] [review]
updates to waitForNotification (default) rev1.0

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

Sorry I didn't get to this earlier, but I was really waiting for a review on my patch. There was no point in checking this if those changes there weren't going to get through. And that is the case now. 
Please wait for a landing there, so you have the clear steps on what needs to be changed and you don't work in vain.
Attachment #755136 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 4

6 years ago
Yup, thanks Andreea, I just saw that in the other bug. I will wait for that to land.
(Assignee)

Comment 5

5 years ago
I see Andreea has a new patch up for review for bug 758187. I retested her latest patch 2013-08-01 + this patch for the refactoring. These three tests with both patches appear to be working and passing as expected. The syntax used for the watiForNotification method doesn't appear to have changed so far.

I also simulated a failure case with the one restart test and it reported the failure as expected.

Tested in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc4 latest + patch rev1.0
latest Nightly 25.0a1 20130729030214 140267
(Assignee)

Comment 6

5 years ago
The patch still appears valid for landing on default, run with the work landed last week in bug 758187.

Re-tested and passed in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc6 latest + patch rev1.0
latest Nightly 27.0a1 20131001030204 149422
(Assignee)

Comment 7

5 years ago
Created attachment 8339988 [details] [diff] [review]
updates to waitForNotification (default) rev1.1

The mozmill-tests repo has since been restructured in bug 927937. This is a revised patch to represent the new firefox/ metrofirefox/ structure, and another merge for the latest state of the tests in default.

Re-tested and passed in:
Windows XP32 SP3
mozmill-env w/mozmill 2.1-dev latest + patch rev1.1
latest Nightly 28.0a1 20131128030201 157937
Attachment #755136 - Attachment is obsolete: true
Attachment #8339988 - Flags: review?(andreea.matei)
Comment on attachment 8339988 [details] [diff] [review]
updates to waitForNotification (default) rev1.1

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

There's one more instance here:

http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js#l75
Attachment #8339988 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 9

5 years ago
Thanks Andreea, will add it in.
(Assignee)

Comment 10

5 years ago
Created attachment 8343258 [details] [diff] [review]
updates to waitForNotification (default) rev1.2

Additional test added as indicated. I also tried another failure-case (switching a boolean in the test) and the test harness does indicate a failure as expected. As per below:

ERROR | Test Failure | {
  "exception": {
    "message": "Modal dialog has been found and processed",
    "lineNumber": 27,
    "name": "AssertionError",
    "fileName": "resource://mozmill/modules/errors.js"
  }

Assuming that is our expected assert message.

Re-tested and passed in:
Windows XP32 SP3
mozmill-env w/mozmill 2.1-dev latest + patch rev1.2
latest Nightly 28.0a1 20131205030207 158806
Attachment #8339988 - Attachment is obsolete: true
Attachment #8343258 - Flags: review?(andreea.matei)
Comment on attachment 8343258 [details] [diff] [review]
updates to waitForNotification (default) rev1.2

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/adb6928641c7 (default)

Thanks Jonathan!
Attachment #8343258 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox29: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.