972600 - [B2G][Contacts] After merging duplicate contacts selecting edit contact button brings up "Add contact" screen

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Brogan Zumwalt [Inactive]]

Description:
After merging two contacts as duplicates (both contacts have same phone number and similar names) tapping "edit contact" button on the resulting merged contact screen brings up an "Add Contact" page.

Repro Steps:
Prerequisite: Have two contacts with the same phone number and similar names (e.g. "Mar" & "Maria") present in address book. 
1) Updated Buri to BuildID: 20140212004003
2) Open Contacts app
3) Select one of the two available contacts
4) Touch "Find duplicate contacts" button
5) Select "merge"
6) Press "edit contact" button

Actual:
Editing merged duplicate contact brings up "Add contact" screen.

Expected:
Edit contact screen appears when user presses edit contact button.

Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140212004003
Gaia: ce17d5eae7b1893ae4397c814b10ae598fcbdb58
Gecko: ab07e61c2eb0
Version: 28.0
Firmware Version: V1.2-device.cfg

Notes:
Repro frequency: 3/3, 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/9836/
See attached: Youtube video clip - http://youtu.be/iUjcCnRwHJY

Comment 1

[Jason Smith [:jsmith]]

What happens on 1.2?

Comment 2

[pfield]

Tested on today's Buri 1.2 and this behaviour did not occur. What instead occured is that the user, upon merging the duplicate contacts and hitting the edit contact button, was brought to the appropriate edit contact screen with all of the contact info correctly saved. (So I am saying that it appears to work correctly on 1.2)

Environmental Variables:
Device: Buri 1.2 MOZ
BuildID: 20140213004001
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: 4fd893896d02
Version: 26.0
Firmware Version: V1.2-device.cfg

Comment 3

[Jason Smith [:jsmith]]

Okay - so this is a regression that leads to wrong UX.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+ regression

Comment 5

[Germán Toro del Valle (:gtorodelvalle)]

I am sorry but I am not able to reproduce this issue using today's v1.3 (Gecko-0b4f0a8.Gaia-a43904d) :S

Brogan, would you be so kind to retest it?, just in case the issue has been solved or I am doing something wrong regarding the STR. Thanks!

Comment 6

[Germán Toro del Valle (:gtorodelvalle)]

In fact, you'll notice using today's v1.3 build (Gecko-0b4f0a8.Gaia-a43904d) that the "Find duplicates contacts" does not respond to the clicking as it should :-O I am managing this issue with our QA team and I will fill a new bug if appropriate ;-)

Comment 7

[Germán Toro del Valle (:gtorodelvalle)]

Please, do not take comment 6 into consideration it seems it is related to my fingers :-O Not kidding! I am able to reproduce it but the QA team does not :-(

Comment 8

[Michał Budzyński (:michalbe)]

I'm not able to reproduce the bug on today's build as well.

Comment 9

[J Zimbrick]

I was able to reproduce this on the latest Buri 1.3 build.

Out of 10 tries using the steps provided in Comment 0, I saw the issue reproduce 5 times. It seems that the repro rate is now closer to 50%.

Environmental Variables:
Device: Buri
BuildID: 20140219004003
Gaia: a43904d9646109b48836d62f7aa51e499fbf4b2e
Gecko: 97922b6daad1
Version: 28.0
Base Image: V1.2-device.cfg

Re-adding regressionwindow-wanted tag.

Comment 10

[Brogan Zumwalt [Inactive]]

Using same environmental variables as comment 9 on Buri 1.3 Mozilla Ril, I was able to get issue to reproduce 1 out of 3 tries.

Comment 11

[J Zimbrick]

I'm actually able to repro this issue on the earliest available 1.3 Buri build. I saw it on 3 out of 10 tries.

1.3 Environmental Variables:
Device: Buri 
BuildID: 20130919040201
Gaia: 0dedb5b3789afc638a0c7c67652937fcb30e77d2
Gecko: 803189f35921
Version: 27.0a1
Base Image: V1.2-device.cfg

This can also be reproduced on the latest 1.2, though the repro rate in that branch seems much lower. Out of 25 tries following the steps in Comment 0, I only saw the issue reproduce 6 times.

I checked with pfield, he had checked it 3 times on 1.2 and did not see the issue reproduce. Given that the repro rate initially seemed to be much higher it felt like a sufficient amount of tries at the time, which explains the differences in results.

1.2 Environmental Variables:
Device: Buri
BuildID: 20140213004001
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: 4fd893896d02
Version: 26.0
Base Image: V1.2-device.cfg
 
1.1 does not seem to have the merge duplicate contacts feature at all, this may have been introduced with that feature in 1.2.

Removing the regression tags for now.

Comment 12

[Jason Smith [:jsmith]]

(In reply to J Zimbrick from comment #11)
> I'm actually able to repro this issue on the earliest available 1.3 Buri
> build. I saw it on 3 out of 10 tries.
> 
> 1.3 Environmental Variables:
> Device: Buri 
> BuildID: 20130919040201
> Gaia: 0dedb5b3789afc638a0c7c67652937fcb30e77d2
> Gecko: 803189f35921
> Version: 27.0a1
> Base Image: V1.2-device.cfg
> 
> This can also be reproduced on the latest 1.2, though the repro rate in that
> branch seems much lower. Out of 25 tries following the steps in Comment 0, I
> only saw the issue reproduce 6 times.
> 
> I checked with pfield, he had checked it 3 times on 1.2 and did not see the
> issue reproduce. Given that the repro rate initially seemed to be much
> higher it felt like a sufficient amount of tries at the time, which explains
> the differences in results.
> 
> 1.2 Environmental Variables:
> Device: Buri
> BuildID: 20140213004001
> Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
> Gecko: 4fd893896d02
> Version: 26.0
> Base Image: V1.2-device.cfg
>  
> 1.1 does not seem to have the merge duplicate contacts feature at all, this
> may have been introduced with that feature in 1.2.
> 
> Removing the regression tags for now.

The analysis here isn't right. The fact that there was an increase of reproducibility from 6/25 to 100% implies a regression during the 1.3 timeframe. Please get the regression range that focuses on when the reproducibility went from being intermittent to being consistently reproducible.

Comment 13

[Germán Toro del Valle (:gtorodelvalle)]

Always scary when it seems we observe distinct things when "supposedly" doing the same :O

To try to narrow that "supposedly" down, would you you be so kind to share the concrete data you are using and which part are you repeating to get that reproducibility rate? I guess you just click on the "edit contact icon" and get the "Edit contact" ("Add contact" some times), close that page, click on the "edit contact icon" and see what you get, close that page, etc. Or do you create the second contact and merge the contacts again as part of the repetition?

Using today's v1.3 build (Gecko-7c32fd6.Gaia-a43904d) and following the next steps, this is what I get:
  1. Create a contact with name "Mar" and phone number "666666666".
  2. Create a contact with name "Maria" and phone number "666666666".
  3. The duplicate found page is shown.
  4. Close the "duplicate found" page clicking on the "X" icon.
  5. Click on "Mar" to get her contacts details.
  6. Click on "Find duplicate contacts".
  7. "Maria" is found as a duplicate contact.
  8. Click on "Merge with 1 contact".
  9. "Mar" contact details page is shown.
  10. Click on the "edit contact icon".
  11. The "Edit contact" page is shown.
  12. Close the "Edit contact" page.
  13. GOTO 10.

This is, I get the "Edit contact" page 100% of the cases. I guess we are doing something differently here.

Comment 14

[Loli (:lolimartinezcr)]

Tested (02/20/2014) and working 
1.3
Gecko 7c32fd6
Gaia a4304d

Comment 15

[Brogan Zumwalt [Inactive]]

Re: Comment 13

I believe we are following the same STR on our side with some exceptions:

On your step 13, we are deleting created contacts and going back to step 1. This also means that the contact list has no existing contacts at step 1. I have yet to reproduce this issue once it has occurred from your step 10 on the same contact.

Comment 16

[Brogan Zumwalt [Inactive]]

Re-tested this issue on the 1.3 build listed in Comment 0 as repro rate seems to be of major concern regarding this bug. Was no longer able to get 100% repro rate experienced when bug was filed. Rate now seems to be in line with Comment 9, Comment 10, and Comment 11 using build 20140212004003.

Further investigation revealed that restarting the phone in between attempts may improve repro rate (observed 4/7; 57%)

Comment 17

[Brogan Zumwalt [Inactive]]

Attachment: Logcat

Attached logcat of issue occurring using environmental variables in Comment 0

Comment 18

[Preeti Raghunath(:Preeti)]

What's the impact to the user here?

It doesn't seem easily reproducible so will need to unblock on 1.3 based on user impact.

Comment 19

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #18)
> What's the impact to the user here?
> 
> It doesn't seem easily reproducible so will need to unblock on 1.3 based on
> user impact.

The impact is entirely wrong UX, which is likely to confuse someone significantly. The reproducibility is high enough that someone is likely to hit (57%), so I tend to disagree that this is safe to unblock.

Comment 20

[Jason Smith [:jsmith]]

Can we re-clarify the STR here & reproduction rate? I'm a bit lost in the confusion of the comments here.

Comment 21

[Germán Toro del Valle (:gtorodelvalle)]

I managed to reproduce it on my third attempt \o/

Let me try to clarify the STR here:

  0. Delete the contacts with name "Mar" and "Maria" and phone number "666666666" if they already exist.
  1. Create a contact with name "Mar" and phone number "666666666".
  2. Create a contact with name "Maria" and phone number "666666666".
  3. The duplicate found page is shown.
  4. Close the "duplicate found" page clicking on the "X" icon.
  5. Click on "Mar" to get her contacts details.
  6. Click on "Find duplicate contacts".
  7. "Maria" is found as a duplicate contact.
  8. Click on "Merge with 1 contact".
  9. "Mar" contact details page is shown.
  10. Click on the "edit contact icon".
  11. The "Edit contact" page is shown.
  12. Close the "Edit contact" page.
  13. GOTO 0.

As I said, in my case I reproduced it the third time I got to 11. In that case, I saw the "Add contact" page as the bug claims ;-)

Comment 22

[Germán Toro del Valle (:gtorodelvalle)]

First off, congrats to Brogan for detecting a pretty hidden run condition in the code ;-)

On the second hand and once I understood the problem, let me provide you with a new STR to increase the reproduction rate to 100% :-) :

--------------------------------------------

STR:
1. Create a contact with name "M" and phone number "666666666".
2. Create a contact with name "Ma" and phone number "666666666".
3. The duplicate found page is shown.
4. Close the "duplicate found" page clicking on the "X" icon.
5. Create a contact with name "Mar" and phone number "666666666".
6. The duplicate found page is shown.
7. Close the "duplicate found" page clicking on the "X" icon.
8. Create a contact with name "Mari" and phone number "666666666".
9. The duplicate found page is shown.
10. Close the "duplicate found" page clicking on the "X" icon.
11. Create a contact with name "Maria" and phone number "666666666".
12. The duplicate found page is shown.
13. Close the "duplicate found" page clicking on the "X" icon.
14. Click on "M" to get her contacts details. If fact any of the created contacts is fine.
15. Click on "Find duplicate contacts".
16. The other 4 contacts are found as duplicate ones.
17. Click on "Merge with 4 contacts".
18. "M" contact details page is shown. Or the clicked contact in 14.
19. Click on the "edit contact icon".

OBSERVED:
20. The "Add contact" page is shown.

EXPECTED:
20. The "Edit contact" page is shown.

--------------------------------------------

To increase the reproduction rate, just increase the number of additional created duplicate contacts before finding the duplicate contacts and merging.

From this on it is a note to myself and for further discussion with José Manuel and Francisco :-) :

The problem was the following. When merging the contact with the other 4 duplicate ones, the contactchange event was notified 5 times: 1 with event.reason 'update' and 4 with event.reason 'remove'. The problem is that the processing of the 'update' event is completed before the last of the 'remove' events is notified and consequently this line of code https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts.js#L747 is executed before this one https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts.js#L769. The final consequence is that |currentContact| is left as {} instead as the updated/merged contact and when clicking on the 'edit contact icon' there's no contact data to load into the form.

A possible quick solution to the problem could be to delete this line https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts.js#L769 I have tested it and I haven't found any regressions but it is a risky move since we may be missing some buggy scenarios :-(

I will discuss it with José Manuel and Francisco to try to find the best solution to the problem.

Comment 23

[Francisco Jordano [:arcturus] [:francisco]]

Fantastic analysis.

To be sure we have no problem removing that line, the detail view has a method to setup the contact it's displaying.

We can do that when we receive an update and we are in the detail view (like is the case).

Then when we call:
|Contacts.showForm(true);| to display the edit form, we could add an extra parameter to indicate what's the current contact:
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/details.js#L92
(There we can tell the Contact class what's the current contact)

Unfortunately this is cause the way we were using the Contacts class a a hub for navigating. Something we need to change with haida for sure.

Cheers.

Comment 24

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 16619.html

Thanks Francisco! ;-)

I finally took Francisco's comment as a source of inspiration as a way to include as least changes as possible to cover the buggy scenario. As a consequence, the line I was proposing to delete when the 'remove' event was notified is not deleted. In fact, the only changes are to pass the updated/merged contact to the contacts.Details object when the 'update' event is notified and the contacts details view is visible (as Francisco suggests). This contact is passed back to the Contacts main object when the form has to be visualised.

Comment 25

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8381482 [details]
16619.html

Hi German, thanks for the code, taking into account the current landing policy, could you provide some unit tests before Rick realizes the PR doesn't contain them ;) ?

Thanks!

Comment 26

[Anthony Ricaud (:rik)]

(I did notice it :D )

Comment 27

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8381482 [details]
16619.html

Hi guys!!! After fighting against the Marionette JS Runner :p I managed to create a passing integration test for the case reported in this bug. In fact, I have just confirmed that it does not pass in current master and it does pass when my patch is applied so I am happy about it ;-)

Francisco, would you be so kind to re-review the patch? Thanks!

Comment 28

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8381482 [details]
16619.html

Perfect \o/

Thanks German, I'm just rerunning one unit test in travis to have all things green!

Comment 29

[Francisco Jordano [:arcturus] [:francisco]]

Landed:

https://github.com/mozilla-b2g/gaia/commit/db3b537b4a29be4849f45fb7ec27b77bb778a2a6

Comment 30

[Francisco Jordano [:arcturus] [:francisco]]

Asking for verify to land this in 1.3

Comment 31

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8381482 [details]
16619.html

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Run condition when merging "many" (around 4-5) contacts. The deletion of contacts was deleting information needed to show the merged contact details.
[User impact] if declined: Bad user experience. User may feel lost since the shown "Add contact" page has no sense.
[Testing completed]: Integration test for merging 15 distinct duplicate contacts included in the patch.
[Risk to taking this patch] (and alternatives if risky): Low. Locally tested by the Telefónica QA team and a integration test covering the buggy case is included.
[String changes made]: None

Comment 32

[Kevin Grandon :kgrandon]

Sorry, I've had to revert this due to causing an intermittent test failure on travis.

The reverted commit: https://github.com/mozilla-b2g/gaia/commit/3e6b11d903d861fbc102d8d269cbf22e4a80c05c

The details of the travis failure:

1) Contacts > Details Merging 15 contacts:

Error: timeout exceeded!

at Object.Client.waitForSync (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:679:16)

at Object.Client.waitFor (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:648:60)

at Object.Contacts.waitSlideLeft (/home/travis/build/mozilla-b2g/gaia/apps/communications/contacts/test/marionette/lib/contacts.js:110:17)

at Context.<anonymous> (/home/travis/build/mozilla-b2g/gaia/apps/communications/contacts/test/marionette/details_test.js:65:13)

at Test.Runnable.run (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:211:32)

at Runner.runTest (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:372:10)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:448:12

at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:297:14)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:307:7

at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:245:23)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:269:7

at Hook.Runnable.run (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:213:5)

at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:257:10)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:269:7

at done (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:185:5)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:197:9

at Object.executeHook (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:370:18)

at process._tickCallback (node.js:415:13)

Comment 33

[Roland Kunkel [:RolandK]]

This issue does not occur in the latest v1.4 build.

Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140304040200
Gaia: 6df4533f14ec2645bb13d8a803a5151583ca13a8
Gecko: 529b86b92b1d
Version: 30.0a1
Firmware: V1.2-device.cfg

Comment 34

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 16910.html

Hi Kevin, thanks for the info ;-)
The point is that there is no clear cause of the intermittent failures to me :-( The code seems legit. Anyhow, I "improved" the already existent code in /marionette/lib/contacts.js in case that was the cause of the issue. Some examples of those improvements are extracting the finding of Marionette elements from the waitFor() invocations (please see https://github.com/mozilla-b2g/gaia/pull/16910/files#diff-93f450be0058b99f97a02c6657b29880R110) as well as using <= instead of the === operator (see https://github.com/mozilla-b2g/gaia/pull/16910/files#diff-93f450be0058b99f97a02c6657b29880R113).
On the other hand, I really do not know how to check if the intermittent failures persist since every time I run the integration tests locally in my laptop they pass. What do you suggest? Thanks!

Comment 35

[Kevin Grandon :kgrandon]

Hey - have you tried running them on travis because it's a slower machine? 

I would recommend running it 100 times when it's slow to see if there's any intermittent failures after your patch. You can use something like this in a pull request: https://gist.github.com/KevinGrandon/8047042

Also you may want to add a bit of logging during the waitFor() changes to ensure that we are waiting properly for all transitions.

Comment 36

[Kevin Grandon :kgrandon]

Comment on attachment 8386212 [details]
16910.html

If you can show me 50+ green travis runs in a job, I will gladly R+ this. Thanks!

Comment 37

[Francisco Jordano [:arcturus] [:francisco]]

Hei,

just an idea, why don't you enable travis in your own repo, and we run this let's say not 50 times but 10?
Kevin will you like that solution? So we could try in the same kind of environment.

This is a 1.3+ bug, and despite that I agree with Kevin about don't landing buggy tests, we need the proper fix :_)

Thanks folks!

Comment 38

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8386212 [details]
16910.html

LGTM, please follow Kevin's advice, let's run this several times in your repo and let's check if we get several greens in a row :)

Comment 39

[Kevin Grandon :kgrandon]

I'm fine with that - TBH 10 times is not enough. If we have a 5% failure rate, it's likely that it won't show up in that run. Ultimately I leave it up to the reviewier - but if the test has an intermittent failure anytime within the next week it will probably be backed out - oranges really hurt us so we need to do everything we can to avoid them. (Much less ideal, but if it's necessary for 1.3 I suppose we could spin the tests off into a follow-up bug and land the original fix still).

Comment 40

[Francisco Jordano [:arcturus] [:francisco]]

@German can we make it 50?

Not a hard request, but Kevin will be happier here (and me too to be honest). Now that you can run it against your fork (and don't compete with other instances), we can give it a try isnt?

Thanks!

Comment 41

[Germán Toro del Valle (:gtorodelvalle)]

Hi again! I made it 20 :-) https://travis-ci.org/gtorodelvalle/gaia/builds/20276340 As you can see there are 3 failing runs but the failing tests are not related to any of the updates included in this patch :-)

For some strange reason I do not see the result of Travis in the PR (attachment 8386212 [details]) :-( I guess it is a matter of time :-)

Comment 42

[Germán Toro del Valle (:gtorodelvalle)]

Ups, forgot the ni :-(

Comment 43

[Germán Toro del Valle (:gtorodelvalle)]

Uhmmm... I have an issue with the nis... :-) (yeah, I am using Chrome on this laptop and these are the consequences... :p)

Comment 44

[Germán Toro del Valle (:gtorodelvalle)]

I just launched 50 integration test runs as requested by José Manuel... :-) I'll come back to you with the results ;-) Thanks!

Comment 45

[Kevin Grandon :kgrandon]

Not sure if there's anything relevant for me here, but please let me know if there is - and how the test run goes. Thanks!

Comment 46

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! And finally we have a winner :-) As you can see at https://travis-ci.org/gtorodelvalle/gaia/builds/20514078 , I ran 50 integration test runs in a row and although there are many which are failing, none of them is related to the updates included in this patch. Anyhow, it would be great if you could check it too, please.

Ready to merge attachment 8386212 [details], José Manuel?

Comment 47

[Kevin Grandon :kgrandon]

Glad your tests passed! That is way too much red for typical use though, so I'm going to go through and disable/back out some of the newer tests. Thanks for handling this!

Comment 48

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8386212 [details]
16910.html

Nice, 50 executions and working well ;)

Comment 49

[Germán Toro del Valle (:gtorodelvalle)]

Thanks Francisco ;-)

Merged in master: https://github.com/mozilla-b2g/gaia/commit/0b7491cc15bb1a8b6de425d2cc23e7d5b474593d

Comment 50

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8386212 [details]
16910.html

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Run condition when merging "many" (around 4-5) contacts. The deletion of contacts was deleting information needed to show the merged contact details.
[User impact] if declined: Bad user experience. User may feel lost since the shown "Add contact" page has no sense.
[Testing completed]: Integration test for merging 15 distinct duplicate contacts included in the patch.
[Risk to taking this patch] (and alternatives if risky): Low. Locally tested by the Telefónica QA team and a integration test covering the buggy case is included.
[String changes made]: None

Comment 51

[Ryan VanderMeulen [:RyanVM]]

v1.3: bfc6b263d296bd32422164d8f42097a4dc52c39c

Comment 52

[Kevin Grandon :kgrandon]

*cry* 

 1) Contacts > Details Merging 15 contacts:

Error: timeout exceeded!

at Object.Client.waitForSync (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:679:16)

at Object.Client.waitFor (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:648:60)

at Object.MarionetteHelper.waitForElement (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-helper/index.js:131:12)

at Object.Contacts.waitForFormShown (/home/travis/build/mozilla-b2g/gaia/apps/communications/contacts/test/marionette/lib/contacts.js:130:35)

at Object.Contacts.enterContactDetails (/home/travis/build/mozilla-b2g/gaia/apps/communications/contacts/test/marionette/lib/contacts.js:159:10)

at Object.Contacts.addContact (/home/travis/build/mozilla-b2g/gaia/apps/communications/contacts/test/marionette/lib/contacts.js:180:10)

at Context.<anonymous> (/home/travis/build/mozilla-b2g/gaia/apps/communications/contacts/test/marionette/details_test.js:43:15)

at Test.Runnable.run (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:211:32)

at Runner.runTest (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:372:10)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:448:12

at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:297:14)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:307:7

at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:245:23)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:269:7

at Hook.Runnable.run (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:213:5)

at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:257:10)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:269:7

at done (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:185:5)

at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:197:9

at Object.executeHook (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:370:18)

at process._tickCallback (node.js:415:13)

Comment 53

[Germán Toro del Valle (:gtorodelvalle)]

Geez!!! :-) I do not know if these intermittent failures may have something to do with this: https://bugzilla.mozilla.org/show_bug.cgi?id=944322 Not probably the best solution but do you want me to increase the element searching timeout? Anyhow, right now it is set to 10000 and consequently I wouldn't bet this is the cause of the issue.

According to Kevin's logs, the issue is related to the lazy loading templates (more concretely, https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/index.html#L178) which Marionette seems not to be a fan of :-) What I will do next is instead of waiting for the concrete lazy loading section (##view-contact-form) to be included in the DOM and displayed (since it already is but its contents are not) I will include a condition to wait for the section's header title to check when the lazy loaded section contents are really available :s

I'll come back to you with the results ;-)

The issue does not seem to have anything to do with Kevin's patch to solve the JSHint issues on the updated files: https://github.com/mozilla-b2g/gaia/commit/488670bc36e3cff5b2ebc749d48f9dc26897914e :-p

Comment 54

[Michael Henretty [:mikehenrty][:mhenretty]]

This is perma-red on 1.3 in travis:

https://travis-ci.org/mozilla-b2g/gaia/jobs/20718997

 1) Contacts > Details Merging 15 contacts:

TypeError: Cannot read property '1' of null

 at Object.Contacts.l10n (/home/travis/build/mozilla-b2g/gaia/apps/communications/contacts/test/marionette/lib/contacts.js:107:33)

at Context.<anonymous> (/home/travis/build/mozilla-b2g/gaia/apps/communications/contacts/test/marionette/details_test.js:85:34)

Comment 55

[Michael Henretty [:mikehenrty][:mhenretty]]

Backed this out of 1.3 for now until we can figure out comment 54 perma-red.

https://github.com/mozilla-b2g/gaia/commit/943b5132feab56e7e7da6b8bd56bd8ff1c3f110c

Comment 56

[Kevin Grandon :kgrandon]

Should we also back this out of master? It's currently intermittent there as well.

Comment 57

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! There is something weird going on here :-S

Ryan, would you be so kind of pasting the link to the commit which uplifted the patch to v1.3 according to your last comment (sorry but it does not have a link to refer to it).

The weird thing is that the error Michael mentions in comment 54 has no sense if we had uplifted all dependant patches. See https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/marionette/lib/contacts.js#L100 in master versus https://github.com/mozilla-b2g/gaia/blob/v1.3/apps/communications/contacts/test/marionette/lib/contacts.js#L103 in v1.3. That line should have been updated by some commit.

I will uplift the proposed patch to v1.3 locally and run the tests to check which may have been the issue.

Thanks!

Comment 58

[Germán Toro del Valle (:gtorodelvalle)]

First dependency found... ;-)

Comment 59

[Germán Toro del Valle (:gtorodelvalle)]

bug 957497 is the one which updates the l10n function in contacts.js (see https://github.com/mozilla-b2g/gaia/blob/v1.3/apps/communications/contacts/test/marionette/lib/contacts.js#L90) So definitely another dependency.

The point is that this bug's patch includes many other changes which are not needed, so I suggest to prepare a specific PR to v1.3 including all the updates needed to solve the issue this bug reports and of course passing all the tests. What do you think?

Comment 60

[Jose Manuel Cantera]

(In reply to gtorodelvalle from comment #59)
> bug 957497 is the one which updates the l10n function in contacts.js (see
> https://github.com/mozilla-b2g/gaia/blob/v1.3/apps/communications/contacts/
> test/marionette/lib/contacts.js#L90) So definitely another dependency.
> 
> The point is that this bug's patch includes many other changes which are not
> needed, so I suggest to prepare a specific PR to v1.3 including all the
> updates needed to solve the issue this bug reports and of course passing all
> the tests. What do you think?

Yes, I agree with Germán. We need to be very pragmatic here and to opt in for a solution that is a quick win. This bug is blocking and has been there for ages. It is time to fix it and move forward

Comment 61

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #51)
> v1.3: bfc6b263d296bd32422164d8f42097a4dc52c39c

(In reply to gtorodelvalle from comment #57)
> Ryan, would you be so kind of pasting the link to the commit which uplifted
> the patch to v1.3 according to your last comment (sorry but it does not have
> a link to refer to it).

https://github.com/mozilla-b2g/gaia/commit/bfc6b263d296bd32422164d8f42097a4dc52c39c

Comment 62

[Germán Toro del Valle (:gtorodelvalle)]

Ups Ryan, missed that. Thanks! ;)

I am currently running the tests in my laptop including the patch I have prepared for v1.3 and waiting for Travis to be up an running :-)

Comment 63

[Germán Toro del Valle (:gtorodelvalle)]

This is the PR I prepared for v1.3: https://github.com/mozilla-b2g/gaia/pull/17197

The tests are passing in my laptop but we'll have to wait for Travis' final verdict.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

v1.3: 17597e6280b149edbc528f59720acc24a5f5cdb5