Closed Bug 656618 Opened 14 years ago Closed 14 years ago

Split testStartStopPBMode.js into two modules

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: AlexLakatos, Assigned: AlexLakatos)

References

Details

Attachments

(1 file, 2 obsolete files)

File: tests/functional/testPrivateBrowsing/testStartStopPBMode.js This test module consists of three tests: * testEnablePrivateBrowsingMode() - https://litmus.mozilla.org/show_test.cgi?id=16611 * testStopPrivateBrowsingMode() - https://litmus.mozilla.org/show_test.cgi?id=16612 * testKeyboardShortcut() - https://litmus.mozilla.org/show_test.cgi?id=16643 I think we should have only one test testStartStopPrivateBrowsingMode() to cover https://litmus.mozilla.org/show_test.cgi?id=16612. testKeyboardShortcut() should be moved to testKeyboardShortcut.js and https://litmus.mozilla.org/show_test.cgi?id=16611 should be disabled. What do you guys think?
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
Blocks: 627975
The test 16611 contains way more information as 16612 and has also been updated without giving us a notice about it. So I would tend to say get 16612 disabled. Otherwise I agree for the keyboard shortcut. Anthony?
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/13336471
Henrik Skupin changed story state to unstarted in Pivotal Tracker
I'm okay with this proposal, although... I'm just not sure simply testing a keyboard shortcut warrants its own test module. Why can't we just run the test once invoking using the UI, and a second time invoking using the keyboard? For that matter, why can't we do this with other tests too?
Alex Lakatos changed story state to started in Pivotal Tracker
(In reply to comment #4) > I'm just not sure simply testing a keyboard shortcut warrants its own test > module. Why can't we just run the test once invoking using the UI, and a > second time invoking using the keyboard? For that matter, why can't we do > this with other tests too? Please clarify that with the Litmus test creator. I think in this case it is Marcia.
Marcia, it would be great if we could get the open question solved by today. Would be nice if you could reply and/or if possible do the update of the Litmus test if agreed on.
I think in the case of these tests that the first one (16611) is comprehensive enough to cover going in and out of PB mode. So we can disable 16612 in Litmus, and I can do that. As far as the test for keyboard shortcuts, in the past on the mac key mapping has been completely broken between Mac OS releases so that is why we have these tests. I would prefer to keep https://litmus.mozilla.org/show_test.cgi?id=16643 in Litmus.
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #532933 - Flags: review?(anthony.s.hughes)
Comment on attachment 532933 [details] [diff] [review] patch v1.0 Can you please add a check to testKeyboardShortcut to test exiting Private Browsing?
Attachment #532933 - Flags: review?(anthony.s.hughes) → review-
Attached patch patch v2.0 (obsolete) — Splinter Review
Added: > //Private Browsing should be stopped > controller.assert(function() { > return !pb.enabled; > }, "Private Browsing Stopped- got '" + !pb.enabled + "'expected 'TRUE'"); Is this enough of a verification or should i do something else?
Attachment #532933 - Attachment is obsolete: true
Attachment #533277 - Flags: review?(anthony.s.hughes)
Comment on attachment 533277 [details] [diff] [review] patch v2.0 >+ //Private Browsing should be stopped >+ controller.assert(function() { >+ return !pb.enabled; >+ }, "Private Browsing Stopped- got '" + !pb.enabled + "'expected 'TRUE'"); Looks fine, just one small nit. > !pb.enabled + "'expected The above should be have a comma like below: > !pb.enabled + "', expected r+ with that change. Over to Henrik for final review.
Attachment #533277 - Flags: review?(hskupin)
Attachment #533277 - Flags: review?(anthony.s.hughes)
Attachment #533277 - Flags: review+
Comment on attachment 533277 [details] [diff] [review] patch v2.0 >+++ b/tests/functional/testPrivateBrowsing/testKeyboardShortcut.js [..] >+ // Create Private Browsing instance and set handler >+ pb = new privateBrowsing.privateBrowsing(controller); >+ pb.handler = pbStartHandler; We should only use the handler in the start/stop test. Otherwise we want to avoid modal dialogs as much as possible. If we have one test which covers that part, it's fairly enough. >+function testKeyboardShortcut() { >+ // Make sure we are not in PB mode and show a prompt >+ pb.enabled = false; >+ pb.showPrompt = true; Just set showPrompt to false, and no modal dialog will appear. >+ // Stop the Private Browsing mode via the keyboard shortcut >+ pb.stop(true); >+ >+ //Private Browsing should be stopped >+ controller.assert(function() { >+ return !pb.enabled; >+ }, "Private Browsing Stopped- got '" + !pb.enabled + "'expected 'TRUE'"); We even don't need this assert. The check is already performed by .stop() which calls waitForTransistionComplete: http://hg.mozilla.org/qa/mozmill-tests/file/c02aa1c2a146/lib/private-browsing.js#l206 >+++ b/tests/functional/testPrivateBrowsing/testStartStopPBMode.js Otherwise looks good. Should be final after the next round. Thanks.
Attachment #533277 - Flags: review?(hskupin) → review-
Attached patch patch v3.0Splinter Review
Attachment #533277 - Attachment is obsolete: true
Attachment #533597 - Flags: review?(hskupin)
Attachment #533597 - Flags: review?(hskupin) → review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: