956668 - [marionette-helper] add selectOnOption method to select option

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect)

Description

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

There are some use cases that we may need to select specific option when testing with marionette.

Comment 1

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on github

Hi Evan, I just created a new method for this case. Can you take a look at this change ? THanks :)

Comment 2

[Evan Tseng [:evanxd]]

Hi EJ,

Look good.
But we need the documentation and tests for the function.
And make sure all tests pass for the patch on Travis.

About the function interface and function name, how do you think we do
```
/**
 * Select a item on selector.
 *
 * @param {Object|String} element a marionette element or a query selector string.
 * @param {String} value the value of item we want to select.
 */
selectOnSelector: function(element, value) {}
```

Please set review again once they are good.
Thanks.

Comment 3

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Ok thanks Evan, but I prefer changing `value` to `index` because we may have the same value and it will make marionette confused and select the wrong element. 

What do you think :D ?

Comment 4

[Evan Tseng [:evanxd]]

Hi EJ,

Just like we discussed.
We do the 'value' way. :)

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Evan !

I just updated the code and try to dump more information for debugging. You can check here : https://travis-ci.org/mozilla-b2g/marionette-helper/jobs/16660619

It seems that the element is not able to be located (even if we already switched to different frame). After digging more deeper, I found helper.waitForAlert is doing the same thing bug got skipped by Gaye. (https://github.com/mozilla-b2g/marionette-helper/commit/ad8b9625e71c667c4b5b67ad55f49f7bf10136b6).

But, I noticed Gaye skipped the test on "marionette-helper" but still use it on Gaia email app when doing marionette test. (https://github.com/mozilla-b2g/gaia/pull/13011/files#diff-6008f82d08377d2aad82307ecc9f53faR102)

To test more, I removed the skip part for helper.waitForAlert and it indeed ran as what I thought - I got the same error `NoSuchElement` like what I got from helper.selectOnSelector

If things go as what I thought, there must be some difference between Gaia and marionette-helper testing environment on Travis to make test not work as expected.

Any ideas !? :)

Comment 6

[Evan Tseng [:evanxd]]

Hi EJ,

As we discussed face to face, we need the screenshot things to figure out what is happened with the tests in Travis.

Thanks. :)

Comment 7

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

This patch focuses adding a new helper function called `selectOnSelector` which can make developers easily select specific selector and choose needed options on system app.

After doing a lot of tests with Evan, we still can't solve why Travis keeps failing and I'll describe what we found below. 

1. it works on my local. (boooooo)

2. based on error message on Travis, we are not able to find specific element until timeout -> we try to add timeout on Makefile but has no luck because problems still exist.

3. There was once (yeah, only once) that Travis passed my test, while the others keeps failing. -> dose this mean that the test work ?

I am not familiar with Marionette test. If there is something missing, please comment below to make me know. Thanks ! 

4.

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

James & Gareth,

can you give me some directions about this patch on marionette-helper ? Thanks :)

Comment 9

[James Lal [:lightsofapollo]]

Gareth is in Taipei this week so he is probably a better candidate :) lemme know if you need anything

Comment 10

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

Hi EJ,

I left some comments on your pull request. I might go ahead and cherry-pick your changeset and apply my suggestions to see if they fix things.

We can also chat tomorrow if you're in the Taipei office.

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Gareth Aye [:gaye] from comment #10)
> Hi EJ,
> 
> I left some comments on your pull request. I might go ahead and cherry-pick
> your changeset and apply my suggestions to see if they fix things.
> 
> We can also chat tomorrow if you're in the Taipei office.

yeah for sure ! I am in Taipei office. Just noticed that you also had timeout issue like me. Maybe we can discuss more about what's going on there in Travis :)

Comment 12

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

Yeah this is a tough one! My current bet is that we aren't focusing on the selector when we tap on the select element. http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.jsm#187 is the code that issues the mozChromeEvent that we listen for (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/value_selector/value_selector.js#L26). Perhaps because we're in Xvfb?

This is a simplified version of EJ's patch https://github.com/mozilla-b2g/marionette-helper/pull/17. Needinfo'ing janjongboom and timdream who contributed to the value selector code to see if they have any ideas.

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

So the problem is that the select element does not have focus after clicking on it?

Comment 14

[Evan Tseng [:evanxd]]

Hi Jan,

Yrs, I think so.

Comment 15

[Evan Tseng [:evanxd]]

^^Yes

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I've no idea; have you loop Rudy?

Comment 17

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Gareth Aye [:gaye] from comment #12)
> Yeah this is a tough one! My current bet is that we aren't focusing on the
> selector when we tap on the select element.
> http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.
> jsm#187 is the code that issues the mozChromeEvent that we listen for
> (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> value_selector/value_selector.js#L26). Perhaps because we're in Xvfb?
> 
> This is a simplified version of EJ's patch
> https://github.com/mozilla-b2g/marionette-helper/pull/17. Needinfo'ing
> janjongboom and timdream who contributed to the value selector code to see
> if they have any ideas.

Interesting !

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Loop Yuren in this thread ! 

He is asking Travis-CI whether we can get the copy image of the test environment and if yes, we will try to setup a local testing server on Taipei office to verify our considerations.

Comment 19

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Per an offline discussion, would like EJ's help to check on a Travis-similar environment if the mozChromeEvent for value selector would be received or not.

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Gareth Aye [:gaye] from comment #12)
> Yeah this is a tough one! My current bet is that we aren't focusing on the
> selector when we tap on the select element.
> http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.
> jsm#187 is the code that issues the mozChromeEvent that we listen for
> (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> value_selector/value_selector.js#L26). Perhaps because we're in Xvfb?
> 
> This is a simplified version of EJ's patch
> https://github.com/mozilla-b2g/marionette-helper/pull/17. Needinfo'ing
> janjongboom and timdream who contributed to the value selector code to see
> if they have any ideas.

Yes, Gareth is right. I have updated my patch to use customEvent and it works finally !! Maybe we need to ask some Travis guru to figure out what's going on when testing on xvfb.

Comment 21

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8356047 [details] [review]
patch on github

Evan please help me review my patch again, thanks ! :)

Comment 22

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

On the one hand... hooray! On the other hand... oh god...

Comment 23

[Evan Tseng [:evanxd]]

Comment on attachment 8356047 [details] [review]
patch on github

Hi 小龍哥,

I already left comments for you patch.
Feel free to set r? once you fix the patch.
Thanks. :)

Comment 24

[Evan Tseng [:evanxd]]

And could you make sure the test could be passed on Travis at least 30 times?
Because we need to make sure the tests are stable enough.
Thanks. :)

Comment 25

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #24)
> And could you make sure the test could be passed on Travis at least 30 times?
> Because we need to make sure the tests are stable enough.
> Thanks. :)

Ok, thanks Evan. I am working on adding a new fake app for this test.

Comment 26

[Evan Tseng [:evanxd]]

We could do matrix in .travis.yml to do test for multiple times.

Comment 27

[Evan Tseng [:evanxd]]

The you could just leave the Travis page here to show that the test is stable.

Comment 28

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8356047 [details] [review]
patch on github

https://travis-ci.org/mozilla-b2g/marionette-helper/builds/17768488

Evan, please help me review again. 

I just passed 30 times on Travis. :P

Comment 29

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Gareth, 

can you also help me take a loot at this patch ? Thanks :)

I discussed with Evan yesterday and we thought it would be nice if we can add marionette-apps as its dependency so that we can add more Gaia related helpers. If you have any other opinions, please comment back here and let's discuss more ! Thanks :)

Comment 30

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

Reviewing now! Also I am fine with depending on marionette-apps, but I would just be careful to keep things that are strictly related to app management in marionette-apps.

Comment 31

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

Looks good to me! r=me

Comment 32

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Gareth Aye [:gaye] from comment #30)
> Reviewing now! Also I am fine with depending on marionette-apps, but I would
> just be careful to keep things that are strictly related to app management
> in marionette-apps.

Yeah for sure, we definitely should keep marionette-apps related logics back in its original repo. All we allowed to do is use the API exported by it and do our own logics. By the way, thanks for your feedbacks and review, Gareth :)

@Evan, I have rebased the commits and push again to make Travis green. Hope this time we can pass the review process ! Thanks :)

Comment 33

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch

Hi Evan, this is the new design of tapOnSelect and it passed Travis exam !!!!!

Please help me review this again !! Thanks ;)

Comment 34

[Evan Tseng [:evanxd]]

Comment on attachment 8375341 [details] [review]
patch

Hi EJ,

I left comments for your patch.
Please update for that and then set r? as :evanxd again.
Thanks.

Comment 35

[Evan Tseng [:evanxd]]

Hi EJ,

Just the last little nit to fix, then we could r+ to land. :)

Comment 36

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8375341 [details] [review]
patch

Evan, we are almost there !!!

Comment 37

[Evan Tseng [:evanxd]]

Comment on attachment 8375341 [details] [review]
patch

Hi EJ,

Nice job! :D

Comment 38

[Evan Tseng [:evanxd]]

master: 0d10f0e2f5aad393fa4baa1bc72cad3e673600be