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)
Firefox OS Graveyard
Gaia::UI Tests
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jlorenzo, Assigned: jlorenzo)
References
Details
Attachments
(1 file)
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/messages/test_sms_to_dialer.py If we don't send an SMS, this test can be easily run offline.
Assignee | ||
Updated•9 years ago
|
QA Whiteboard: [fxosqa-auto-s15]
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
QA Whiteboard: [fxosqa-auto-s15] → [fxosqa-auto-s15,s16]
Comment 3•9 years ago
|
||
Hey Johan, sorry for the delay, your PR will be the first thing I'll look into tomorrow :)
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
(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
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #8) > [2] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=2729d6b84004feb0589b07b71d59a9719d325506 There's we are! It has been executed in Gij30: https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/_vs_FHDuTY6jY_XPMiAj7Q/0/public/logs/live_backing.log
Comment 10•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
(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)
Assignee | ||
Updated•9 years ago
|
QA Whiteboard: [fxosqa-auto-s15,s16] → [fxosqa-auto-s15,s16,s17]
Assignee | ||
Comment 12•9 years ago
|
||
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)
Comment 13•9 years ago
|
||
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!
Comment 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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+
Assignee | ||
Comment 18•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8595389 -
Flags: review?(jdorlus)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Keywords: checkin-needed
Comment 19•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Keywords: checkin-needed
Comment 20•9 years ago
|
||
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.
Comment 21•9 years ago
|
||
In master: https://github.com/mozilla-b2g/gaia/commit/8f07673395b2015c97a8a6bc78b0a6ede3b28307
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 22•7 years ago
|
||
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.
Description
•