Closed Bug 1267388 Opened 6 years ago Closed 6 years ago

Refactor password manager test_prompt.html

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s + ---
firefox49 --- fixed

People

(Reporter: Dolske, Assigned: Dolske)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Spinning off from bug 1266618. I did the mentioned refactoring ala prompt tests in bug 1263784 and bug 1265194. It's already 7 parts long, so let's checkpoint this work before getting the rest of the test working on E10S.

This work basically simplifies most of the core test down to a single linear test function, avoiding the jumping around between 3 functions that the old test did. This makes it a lot clearer to see what's happening. This code for checking and manipulating prompts is all E10S ready, allowing bug 1266618 to focus on the further changes needed for supporting E10S with this test.
Review commit: https://reviewboard.mozilla.org/r/48803/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48803/
Attachment #8745060 - Flags: review?(MattN+bmo)
Attachment #8745061 - Flags: review?(MattN+bmo)
Attachment #8745062 - Flags: review?(MattN+bmo)
Attachment #8745063 - Flags: review?(MattN+bmo)
Attachment #8745064 - Flags: review?(MattN+bmo)
Attachment #8745065 - Flags: review?(MattN+bmo)
Attachment #8745066 - Flags: review?(MattN+bmo)
Attachment #8745065 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8745065 [details]
MozReview Request: Bug 1267388 - trivial code move to inline handleLoad, making the test one contigious function. r?MattN

https://reviewboard.mozilla.org/r/48813/#review45617
Attachment #8745060 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8745060 [details]
MozReview Request: Bug 1267388 - move test to mochitest/ dir, add action object. r?MattN

https://reviewboard.mozilla.org/r/48803/#review45627
Comment on attachment 8745061 [details]
MozReview Request: Bug 1267388 - introduce a "state" object to represent actual/expected prompt state. r?MattN

https://reviewboard.mozilla.org/r/48805/#review45629
Attachment #8745061 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8745062 [details]
MozReview Request: Bug 1267388 - Move "action" objects into main test body. r?MattN

https://reviewboard.mozilla.org/r/48807/#review45631
Attachment #8745062 - Flags: review?(MattN+bmo) → review+
Attachment #8745063 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8745063 [details]
MozReview Request: Bug 1267388 - Add additional "state" checks, in preparation for using common prompt checking helper. r?MattN

https://reviewboard.mozilla.org/r/48809/#review45633
Comment on attachment 8745064 [details]
MozReview Request: Bug 1267388 - Use SpawnTask, make handleLoad() a generator. r?MattN

https://reviewboard.mozilla.org/r/48811/#review45615

::: toolkit/components/passwordmgr/test/mochitest/test_prompt.html:300
(Diff revision 1)
>  /*
>   * handleLoad
>   *
>   * Called when a load event is fired at the subtest's iframe.
>   */
> -function handleLoad() {
> +function *handleLoad() {

Nit: I think m-c is using `function* ` instead of `function *`

I see this is getting removed in the next patch…
Attachment #8745064 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8745066 [details]
MozReview Request: Bug 1267388 - Use test helpers from prompt tests. r?MattN

https://reviewboard.mozilla.org/r/48815/#review45635

::: toolkit/components/passwordmgr/test/mochitest/test_prompt.html
(Diff revision 1)
> -  ok(didDialog, "handleDialog was invoked");
>    is(result.value, "xyz", "Checking prompt() returned value");
>  
>    // ===== test 2 =====
> -  testNum++;
>    state = {
>      msg         : "the message",

In the future it would be nice to use separate add_task for each sub-test so they can have their own local scope which should reduce the number of globals making the tests more self-contained.

Thanks
Attachment #8745066 - Flags: review?(MattN+bmo) → review+
Forgot to mention: I did a Try push last night for this.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=880d5dccf606

Looks ok, except for Android. Should probably investigate how to fix that and bug 1267092.
Blocks: 1266618
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #14)

> >    // ===== test 2 =====
> > -  testNum++;
> >    state = {
> >      msg         : "the message",
> 
> In the future it would be nice to use separate add_task for each sub-test so
> they can have their own local scope which should reduce the number of
> globals making the tests more self-contained.

Agreed.

Also, you mentioned the inconsistency of having |handlePrompt(action)| take action as an argument, but not state (although it ends up being used, since it's global). Amusingly, I just noticed that the implementation of handlePrompt doesn't even have an argument, so it's being ignored when passed. We should make that handlePrompt(state, action).
(In reply to Justin Dolske [:Dolske] from comment #15)
> Forgot to mention: I did a Try push last night for this.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=880d5dccf606
> 
> Looks ok, except for Android. Should probably investigate how to fix that
> and bug 1267092.

Actually, that's a failure in test_autofill_password-only.html which I'm not touching. Nor am I changing anything it's imports (or pwmgr itself), so... O_o

Both the 4 original runs and my 4 retriggers are orange, so something's unhappy. Let's see if an empty Try push also fails, to see if it's an existing issue when running just the tests in this dir?
Yeah, so the same test went orange 4x. Something is already broken when running just the pwmgr dir on Android. I'll file a bug.
Filed bug 1267849.
https://hg.mozilla.org/integration/fx-team/rev/c12a44597304350163384f99c9a127bd566d6c50
Bug 1267388 - move test to mochitest/ dir, add action object. r=MattN

https://hg.mozilla.org/integration/fx-team/rev/34c89dcf5311362f87f0313a7e2c6fbc8e541350
Bug 1267388 - introduce a "state" object to represent actual/expected prompt state. r=MattN

https://hg.mozilla.org/integration/fx-team/rev/5c2718553e31e0677974a7eb7b48b275cfc0ffc6
Bug 1267388 - Move "action" objects into main test body. r=MattN

https://hg.mozilla.org/integration/fx-team/rev/f5f2d8448dd3c6213a7c2e09932e8c5464744c0b
Bug 1267388 - Add additional "state" checks, in preparation for using common prompt checking helper. r=MattN

https://hg.mozilla.org/integration/fx-team/rev/a40b1ae6c23074d48adcd5954311b5c3fb5d3415
Bug 1267388 - Use SpawnTask, make handleLoad() a generator. r=MattN

https://hg.mozilla.org/integration/fx-team/rev/f4fa0d9f8d9fad7bfa4fa13ab3265de1abe766fe
Bug 1267388 - trivial code move to inline handleLoad, making the test one contigious function. r=MattN

https://hg.mozilla.org/integration/fx-team/rev/a9b8d706943e258a701d799d9bda85fb79e74f45
Bug 1267388 - Use test helpers from prompt tests. r=MattN

https://hg.mozilla.org/integration/fx-team/rev/48b29eda8f8b295dd6f5cb131547c5c8101c2d17
Bug 1267388 - Make handlePrompt() take state/action args and actually use them. r=MattN
(In reply to Justin Dolske [:Dolske] from comment #21)

> https://hg.mozilla.org/integration/fx-team/rev/
> 48b29eda8f8b295dd6f5cb131547c5c8101c2d17
> Bug 1267388 - Make handlePrompt() take state/action args and actually use
> them. r=MattN

Added this per comment 16, and Matt rubber-stamped it on IRC.
Assignee: nobody → dolske
https://hg.mozilla.org/integration/fx-team/rev/a6688f332ebcb7bc6c634e0bfc03c73fff6f42c6
Bug 1267388 - fix test due to concurrent landing of bug 1268159 which changed the string this test is now checking. DONTBUILD
Blocks: 1298193
Blocks: 1298543
You need to log in before you can comment on or make changes to this bug.