1267388 - Refactor password manager test_prompt.html

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

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.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: MozReview Request: Bug 1267388 - move test to mochitest/ dir, add action object. r?MattN

Review commit: https://reviewboard.mozilla.org/r/48803/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48803/

Comment 2

[Justin Dolske [:Dolske]]

Attachment: MozReview Request: Bug 1267388 - introduce a "state" object to represent actual/expected prompt state. r?MattN

Review commit: https://reviewboard.mozilla.org/r/48805/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48805/

Comment 3

[Justin Dolske [:Dolske]]

Attachment: MozReview Request: Bug 1267388 - Move "action" objects into main test body. r?MattN

Review commit: https://reviewboard.mozilla.org/r/48807/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48807/

Comment 4

[Justin Dolske [:Dolske]]

Attachment: MozReview Request: Bug 1267388 - Add additional "state" checks, in preparation for using common prompt checking helper. r?MattN

Review commit: https://reviewboard.mozilla.org/r/48809/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48809/

Comment 5

[Justin Dolske [:Dolske]]

Attachment: MozReview Request: Bug 1267388 - Use SpawnTask, make handleLoad() a generator. r?MattN

Review commit: https://reviewboard.mozilla.org/r/48811/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48811/

Comment 6

[Justin Dolske [:Dolske]]

Attachment: MozReview Request: Bug 1267388 - trivial code move to inline handleLoad, making the test one contigious function. r?MattN

Review commit: https://reviewboard.mozilla.org/r/48813/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48813/

Comment 7

[Justin Dolske [:Dolske]]

Attachment: MozReview Request: Bug 1267388 - Use test helpers from prompt tests. r?MattN

Review commit: https://reviewboard.mozilla.org/r/48815/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48815/

Comment 8

[Matthew N. [:MattN]]

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

Comment 9

[Matthew N. [:MattN]]

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 10

[Matthew N. [:MattN]]

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

Comment 11

[Matthew N. [:MattN]]

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

Comment 12

[Matthew N. [:MattN]]

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 13

[Matthew N. [:MattN]]

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…

Comment 14

[Matthew N. [:MattN]]

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

Comment 15

[Justin Dolske [:Dolske]]

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.

Comment 16

[Justin Dolske [:Dolske]]

(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).

Comment 17

[Justin Dolske [:Dolske]]

(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?

Comment 18

[Justin Dolske [:Dolske]]

Empty push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be2092621c4b

Comment 19

[Justin Dolske [:Dolske]]

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.

Comment 20

[Justin Dolske [:Dolske]]

Filed bug 1267849.

Comment 21

[Justin Dolske [:Dolske]]

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

Comment 22

[Justin Dolske [:Dolske]]

(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.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cd2ec0f3b561

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed a follow-up to remove the redeclaraction of authMgr (ESLint complained): https://hg.mozilla.org/integration/fx-team/rev/cd2ec0f3b561

Comment 25

[Justin Dolske [: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

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c12a44597304
https://hg.mozilla.org/mozilla-central/rev/34c89dcf5311
https://hg.mozilla.org/mozilla-central/rev/5c2718553e31
https://hg.mozilla.org/mozilla-central/rev/f5f2d8448dd3
https://hg.mozilla.org/mozilla-central/rev/a40b1ae6c230
https://hg.mozilla.org/mozilla-central/rev/f4fa0d9f8d9f
https://hg.mozilla.org/mozilla-central/rev/a9b8d706943e
https://hg.mozilla.org/mozilla-central/rev/48b29eda8f8b
https://hg.mozilla.org/mozilla-central/rev/cd2ec0f3b561
https://hg.mozilla.org/mozilla-central/rev/a6688f332ebc