Closed
Bug 656618
Opened 14 years ago
Closed 14 years ago
Split testStartStopPBMode.js into two modules
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: AlexLakatos, Assigned: AlexLakatos)
References
Details
Attachments
(1 file, 2 obsolete files)
8.41 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•14 years ago
|
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
Comment 1•14 years ago
|
||
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
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?
Comment 6•14 years ago
|
||
(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.
Comment 7•14 years ago
|
||
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.
Comment 8•14 years ago
|
||
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.
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #532933 -
Flags: review?(anthony.s.hughes)
Comment 10•14 years ago
|
||
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-
Assignee | ||
Comment 11•14 years ago
|
||
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 12•14 years ago
|
||
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 13•14 years ago
|
||
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-
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #533277 -
Attachment is obsolete: true
Attachment #533597 -
Flags: review?(hskupin)
Updated•14 years ago
|
Attachment #533597 -
Flags: review?(hskupin) → review+
Comment 15•14 years ago
|
||
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/b6098c93b1fb (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/65511a8d782d (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/cad5986267a1 (beta)
Leaving open for Litmus updates. Please close once done. Thanks.
Assignee | ||
Comment 16•14 years ago
|
||
Updated https://litmus.mozilla.org/show_test.cgi?id=16643
Updated https://litmus.mozilla.org/show_test.cgi?id=16611
Marking RESOLVED FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•