Closed Bug 1156753 Opened 9 years ago Closed 9 years ago

Migrate test_sms_to_dialer.py to Gij

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jlorenzo, Assigned: jlorenzo)

References

Details

Attachments

(1 file)

QA Whiteboard: [fxosqa-auto-s15]
Comment on attachment 8595389 [details] [review]
[gaia] JohanLorenzo:bug-1156753 > mozilla-b2g:master

I think we're getting close to the first cross app test for SMS. I'm not sure "marionette" is the best folder to put a cross-app integration test. I also wrapped the page class defined in Dialer. Not sure this is the best option too. What do you think, Oleg?

I used all the accessors (like messageApp.Thread.message) defined in lib/messages.js to create the "views". John, I think the naming of "view" in this case is not the correct one (bug 1145657). What name do you propose for that?

Do you guys see any other problematic points?
Attachment #8595389 - Flags: review?(jdorlus)
Attachment #8595389 - Flags: review?(azasypkin)
QA Whiteboard: [fxosqa-auto-s15] → [fxosqa-auto-s15,s16]
Hey Johan, sorry for the delay, your PR will be the first thing I'll look into tomorrow :)
Comment on attachment 8595389 [details] [review]
[gaia] JohanLorenzo:bug-1156753 > mozilla-b2g:master

Left feedback at GitHub. I think test view separation is the right thing to do! But as we discussed already this test should be placed somewhere in shared, while test views should be app folders.

Also please rebase on the latest master - we've landed massive rename patch recently :)

Thanks!
Attachment #8595389 - Flags: review?(azasypkin)
Comment on attachment 8595389 [details] [review]
[gaia] JohanLorenzo:bug-1156753 > mozilla-b2g:master

I moved the test in /shared.

I avoided the use of '../../..' by defining a wrapper called "requireFromApp". To be effective, this wrapper should be in its own package (something like 'gaia-test-require'). If you agree with this, I'll do so.

I factorized ThreadMock to deal with MMS.

I left some comments in the PR about the idea of returning the sub-page object inside a page object.

Tell me what you think.
Attachment #8595389 - Flags: review?(azasypkin)
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #5)
> I moved the test in /shared.

Not sure treeherder will run the test. I'll have to check once the results are here: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=0ea17cdcb0860954f8633f68ab1a6a25b1f807db
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #6) 
> Not sure treeherder will run the test.

Hmmm, doesn't seem to run. I likely have to change the paths where to locate the tests.
Somebody already thought about this problem[1], I just put the file in tests/integration/sms/. Let's verify if TH now executes the test[2].

[1] https://github.com/mozilla-b2g/gaia/blob/master/bin/gaia-marionette#L64
[2] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=2729d6b84004feb0589b07b71d59a9719d325506
Comment on attachment 8595389 [details] [review]
[gaia] JohanLorenzo:bug-1156753 > mozilla-b2g:master

Replied to your comments and left more thoughts on Github.

Thanks!
Attachment #8595389 - Flags: review?(azasypkin)
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #8)
> Somebody already thought about this problem[1].

After some investigation, this folder has been introduced for the devtools in bug 986223. Now, I'm not sure using this folder is the best for testing integration between 2 gaia apps. Gareth, do you think it's okay to use: tests/integration/$startApp/$endApp_test.js? Or maybe we could go with: tests/integration/cross-apps/$startApp/$endApp_test.js?
Flags: needinfo?(gaye)
QA Whiteboard: [fxosqa-auto-s15,s16] → [fxosqa-auto-s15,s16,s17]
Comment on attachment 8595389 [details] [review]
[gaia] JohanLorenzo:bug-1156753 > mozilla-b2g:master

I think we're converging to something we both agree with. Tell me if you see any other weaknesses in the architecture :)
Attachment #8595389 - Flags: review?(azasypkin)
Hey Johan,

I've left some more comments at GitHub.

Keeping r? for now, I think we're almost there :) Just ping me whenever you're ready.

Thanks a lot!
Blocks: 1168118
Comment on attachment 8595389 [details] [review]
[gaia] JohanLorenzo:bug-1156753 > mozilla-b2g:master

Okay, looks good to me now, just few minor nits!

Could you please rebase, squash and re-run Messages related Gijs 10x or 20x times to be sure we're free of any intermittents?

Thanks a lot for your work!
Attachment #8595389 - Flags: review?(azasypkin) → review+
Comment on attachment 8595389 [details] [review]
[gaia] JohanLorenzo:bug-1156753 > mozilla-b2g:master

Rebased, squashed and Gij30 has been re-rerun 30 times. Let's wait for the results[1]

Gabriele, I changed a little bit the dialer files for marionette. It's not supposed to break anything. Could you tell me if you agree with them in apps/communications/dialer/test/marionette/lib/dialer.js ? Thanks!

[1] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=1b304e4736b21f2d6a1c409aeaf0c9ab3c2994c5
Attachment #8595389 - Flags: review?(gsvelto)
Alright, in the last treeherder jobs, the patch made fail 2 types of tests:
 * The first kind was due to [1], I forgot to initialize the accessor objects
 * The second was because [2] used a selector which was non-existent in the Accessor object. I think we can fix that in a follow up patch, where we'll implement MessageAccessor. I put a TODO in the code to remind it.

So now, we have these TH jobs[3]. I rerun 30 times Gij{11,12,28} which contain the SMS tests. They are all green!

[1] https://github.com/mozilla-b2g/gaia/pull/29631/files#diff-a3f0ad18f54e1750f7f7ecbbc2356f25R149
[2] https://github.com/mozilla-b2g/gaia/pull/29631/files#diff-6692a94bffbe100d8db6802a98b50fb8R35
[3] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=efa3ae06ef46169cae6e4e6807d259ba9fc1ecf0
Comment on attachment 8595389 [details] [review]
[gaia] JohanLorenzo:bug-1156753 > mozilla-b2g:master

The dialer parts LGTM. I only wish for the day we won't have it under communications/ anymore with all the complication of the multiple entry points.
Attachment #8595389 - Flags: review?(gsvelto) → review+
For the record, I had to de-bit-rot the test due to bug 1127398. Here's the try run with the bitrot fixes (showing all green with 30 tries on Gij{11,12,28}): https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=e4c1bb1f35d31317a2f452ee37fa938b61e315ac

I just fixed the indentation issue raised in the review in comment 17.
Attachment #8595389 - Flags: review?(jdorlus)
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#6YPEH-DJT4mhshAREiKD0A

The pull request failed to pass integration tests. It could not be landed, please try again.
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#V8NYfL0nTkG4iWDOKR4eeQ

The pull request failed to pass integration tests. It could not be landed, please try again.
In master: https://github.com/mozilla-b2g/gaia/commit/8f07673395b2015c97a8a6bc78b0a6ede3b28307
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 1175110
Bug has already been closed. Clearing Need-Infos.
Flags: needinfo?(gaye)
You need to log in before you can comment on or make changes to this bug.