1049916 - Create Firefox Accounts automated testcases to validate simple user authentication

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::UI Tests, defect, P3)

Comment 1

[Richard Pappalardo [:rpapa][:rpappalardo]]

Attachment: git PR

Comment 2

[Sam Penrose]

Jared this is somewhat beyond my Gaia ken; also I'm having trouble getting the tests to finish. Can I ask you to take a look at it?

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Yup, happy to take it.

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey Richard - Have to stop for the night, but I'll get back to reviewing tomorrow.

Comment 5

[Richard Pappalardo [:rpapa][:rpappalardo]]

Thanks, Jared! The comments are very helpful.

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Sorry, Richard, I'll get back to this review tomorrow :-)

Comment 7

[Richard Pappalardo [:rpapa][:rpappalardo]]

Thanks, Jared.  Btw, should I submit changes as I go along or would you prefer I post them in a one batch?

Comment 8

[Richard Pappalardo [:rpapa][:rpappalardo]]

Attachment: git PR

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8474065 [details] [review]
git PR

I'm not sure I should be reviewing changes for this. Gareth feel free to pass the review on if you want.

Comment 10

[Richard Pappalardo [:rpapa][:rpappalardo]]

Hey Jared, I've checked in all the changes you recommended (perhaps with the exception of some error checking for a hung child process).  Would it be easier if I squash everything down at this point?  Still waiting on getting the external modules into gaia-node-modules so it won't be passing on Travis just yet.

Comment 11

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8474065 [details] [review]
git PR

I am in a calendar cave right now. Talk to jlal!

Comment 12

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey Richard,

Actually, I prefer to keep the commits unsquashed until the review is done, as that lets me read the followup commits (which reminds me what we were discussing...). Thanks for asking :-)

I commented in the PR, maybe we can use those example failure scenarios as tests for the server? (Or maybe we can just wait on that till sometime in the future, since this has already been open a really long time.) If we defer it, at least we can open a bug and comment in the code with the bug number.

If this is ready for another pass, r? me here, and I'll take a final look.

Comment 13

[Richard Pappalardo [:rpapa][:rpappalardo]]

Comment on attachment 8470099 [details] [review]
git PR

Howdy Jared,

Thanks.  
> maybe we can just wait on that till sometime in the future, since this has already been open a really long time
If it's OK with you, I'd prefer to get this landed as soon as possible since I have to do some work on tools for loop.  I did, however, find some good resources last night for unit testing the mock server, so my goal is to push that forward afterwards.

Creating: Bug 1056186 for unit tests

Comment 14

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8470099 [details] [review]
git PR

Really nice work, Richard.

I guess we should wait to check this in until the gaia-node-modules PR has landed, but this looks good to me, the marionette tests run locally.

Thanks for all your hard work here!

Comment 15

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Oh, I should add - now is a good time to rebase/squash the commits, and you can add r=6a68 to the commit message.

Comment 16

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hi James,

Mind taking a quick look at the PR that I r+'d above? They are FxA tests and look good to me, but I kinda feel like someone with more familiarity with the marionette framework should take a look, too.

Not a big deal if you don't have time, though.

Cheers,

Jared

Comment 17

[James Lal [:lightsofapollo]]

I am happy to look but will not have time until Friday (re need infoing myself but don't let me be a blocker or ping me on IRC if you really want me to look sooner)

Comment 18

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

No worries, James. I'm happy to just merge this patch as-is; Richard and I have done a couple rounds of feedback, and the code looks good overall (and looks like the other marionette code).

Richard, consider this patch finished, but I will wait until the gaia-node-modules patch lands before I merge this to master. :-)

Comment 19

[Richard Pappalardo [:rpapa][:rpappalardo]]

Thanks, Jared, James!

Comment 20

[James Lal [:lightsofapollo]]

Comment on attachment 8474065 [details] [review]
git PR

(just r+ to the landing of modules part, in the future your reviewer for the code can land the deps if they think the code is fine)

Comment 21

[Richard Pappalardo [:rpapa][:rpappalardo]]

sounds good, thanks

Comment 22

[Richard Pappalardo [:rpapa][:rpappalardo]]

James, I see that you r+'ed this, but looks like the PR is still open on gaia-node-modules.  Were you going to merge it this time, or were you expecting Jared to do it?

Comment 23

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

No worries, Richard, I can merge both.

Comment 24

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Just FYI, Richard, I'm going to wait to merge until gaia-try runs again. I'd like to see if Gij and Gb go green. If not, I might ask you to try rebasing against current master, I'll ni? if so.

Comment 25

[Richard Pappalardo [:rpapa][:rpappalardo]]

Sure thing, Jared.  I'll keep on gaia as well.  If not green by end-of-day, I'll wait for build and most of marionette tests to pass and try rebasing against that.  Thanks for your patience.

Comment 26

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Heh, of course the tests are failing: the PR with the new dependencies hadn't been merged yet.

I've merged that PR, now I'm going to restart the failed tests again.

Comment 27

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey James - Do I need to do anything to get the updated npm dependencies into the gaia-try run? It looks like Gij continues to fail because the new deps aren't getting loaded. Thanks - Jared

Here's the output from the last run: https://tbpl.mozilla.org/?rev=264c199732e9617fc7723ac75da9d07561f55295&tree=Gaia-Try

Comment 28

[James Lal [:lightsofapollo]]

This needs to be updated to point to the particular revision you want in the gaia node modules thing https://github.com/mozilla-b2g/gaia/blob/master/gaia_node_modules.revision

Comment 29

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey Richard,

Would you mind updating your patch to include the new revision for the gaia node modules dependency? See comment 28 for more.

Thanks,

Jared

Comment 30

[Richard Pappalardo [:rpapa][:rpappalardo]]

Not at all.  Was just going to ping you to let you know I'm on it.  I'll commit right after lunch.
Thanks!

Comment 31

[Richard Pappalardo [:rpapa][:rpappalardo]]

OK, thought I did this right, but gaia-try doesn't like it:
Should I be rebasing my gaia branch first, before committing those 2 changes (to avoid merge conflict)?

INFO -  Auto-merging package.json
13:54:03     INFO -  Auto-merging gaia_node_modules.revision
13:54:03     INFO -  CONFLICT (content): Merge conflict in gaia_node_modules.revision
13:54:03     INFO -  Auto-merging .gitignore
13:54:03     INFO -  Automatic merge failed; fix conflicts and then commit the result.
13:54:03    ERROR - Return code: 1

Comment 32

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Yeah, looks like somebody else probably modified the .gitignore file, so you're getting a merge conflict. You could try to manually resolve it, but the simplest way is probably to git fetch and git pull --rebase, then re-push the branch. Maybe grab me on IRC and I can walk you through it.

Comment 33

[Richard Pappalardo [:rpapa][:rpappalardo]]

Looks like no more merge error and gaia-try success on Linux, though still failing on B2G Desktop OS X Opt.  Is there something further I should do?  Perhaps force push again once gaia is green? (probably by the time we retire :)
https://tbpl.mozilla.org/?rev=111fecb4e43f90052858ec050633fe6c6cec1b5b&tree=Gaia-Try

Comment 34

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey Richard,

Hmm. Looking at other tests on gaia-try (you can see them by clicking the little green down arrow in the middle of the screen), it seems that same test fails quite often, but intermittently. In fact, it looks like there's a bug filed for those intermittent failures[1], it just isn't linked to the gaia-try results yet, for whatever reason.

So, I think we're good to go! .... Just one more thing:

Looks like your branch needs to be rebased, it's conflicting with master. We might be better off waiting a few days, since today is a big deadline for 2.1 features, and hg/tbpl have been crashy this week.
  Otherwise, if you want to try to land it today, rebase the branch against master, force-push, make sure github doesn't complain that the branch can't be merged automatically, and then set the checkin-needed keyword (I will be offline to get some work done this afternoon, one of the sheriffs can merge it).

Congrats! We're *almost* there :-)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1060223

Comment 35

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Doh, I marked it as fixed but wasn't able to merge. Reopening.

Comment 36

[Richard Pappalardo [:rpapa][:rpappalardo]]

Sweet!  Thanks, Jared.  No, I won't rush it along.  I'll try again on Tues.  Have a great long weekend!

Comment 37

[Richard Pappalardo [:rpapa][:rpappalardo]]

Hey Jared, I rebased but looks like now I have quite a few files to merge.  Starting working on that, but just want to make sure I haven't hosed my branch.  I'm wondering if maybe there's a more straightforward way to do this?

Comment 38

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hi Richard,

Looking at the branch right now[1], it looks like you did a "git pull", not a "git pull --rebase". So it looks like all the recent changes have been merged on top of your patch, which is not what we want. The branch isn't mergeable in its current state, I wouldn't bother trying to resolve the conflicts.

Here's what I'd suggest:
1. get an up-to-date copy of the mozilla-b2g master branch
2. cherry-pick your commit onto it: "git cherry-pick 20eb14ee8" (assuming this[2] is the right commit - if not, use the SHA from the correct commit)
3. cherry-picking might create some conflicts, so resolve those normally. the only conflicts should be in files you have edited--if you see random other files, something's up.
4. force-push the resulting branch over top of the existing branch

That should fix it. Let me know if it doesn't :-)

[1] https://github.com/rpappalax/gaia/commits/bug-1049916-fxa-auto
[2] https://github.com/rpappalax/gaia/commit/20eb14ee8

Comment 39

[Richard Pappalardo [:rpapa][:rpappalardo]]

Yep, I did.  Thanks for settin me straight. Sorry you had to school me on this, but hopefully all the ducks are in a row now :)
https://tbpl.mozilla.org/?rev=be1d35b3c1c5c8776ff8d7a9f3263f98976dd7fd&tree=Gaia-Try

Comment 40

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Richard Pappalardo from comment #39)
> Yep, I did.  Thanks for settin me straight. Sorry you had to school me on
> this, but hopefully all the ducks are in a row now :)
> https://tbpl.mozilla.org/
> ?rev=be1d35b3c1c5c8776ff8d7a9f3263f98976dd7fd&tree=Gaia-Try

Ha, no worries at all, man. Happy to help ^_^

Looks green to me, so, I'll merge it :-)

Comment 41

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Master: https://github.com/mozilla-b2g/gaia/commit/21c2f4b81ef43cdb47e1c7703bb0266d6a17603a

Congrats!