[marionette-helper] add selectOnOption method to select option

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
9 months ago

People

(Reporter: eragonj, Assigned: eragonj)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

There are some use cases that we may need to select specific option when testing with marionette.
Created attachment 8356047 [details] [review]
patch on github

Hi Evan, I just created a new method for this case. Can you take a look at this change ? THanks :)
Attachment #8356047 - Flags: review?(evanxd)

Comment 2

5 years ago
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.

Updated

5 years ago
Attachment #8356047 - Flags: review?(evanxd)
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

5 years ago
Hi EJ,

Just like we discussed.
We do the 'value' way. :)
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 !? :)
Flags: needinfo?(evanxd)

Comment 6

5 years ago
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. :)
Flags: needinfo?(evanxd)
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.
James & Gareth,

can you give me some directions about this patch on marionette-helper ? Thanks :)
Flags: needinfo?(jlal)
Flags: needinfo?(gaye)
Gareth is in Taipei this week so he is probably a better candidate :) lemme know if you need anything
Flags: needinfo?(jlal)
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.
Flags: needinfo?(gaye)
(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 :)
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.
Flags: needinfo?(timdream)
Flags: needinfo?(janjongboom)
So the problem is that the select element does not have focus after clicking on it?
Flags: needinfo?(janjongboom)
Hi Jan,

Yrs, I think so.
^^Yes
I've no idea; have you loop Rudy?
Flags: needinfo?(timdream) → needinfo?(rlu)
(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 !
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.
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.
Flags: needinfo?(rlu)
(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 on attachment 8356047 [details] [review]
patch on github

Evan please help me review my patch again, thanks ! :)
Attachment #8356047 - Flags: review?(evanxd)
On the one hand... hooray! On the other hand... oh god...
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. :)
Attachment #8356047 - Flags: review?(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. :)
(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.
We could do matrix in .travis.yml to do test for multiple times.
The you could just leave the Travis page here to show that the test is stable.
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
Attachment #8356047 - Flags: review?(evanxd)
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 :)
Flags: needinfo?(gaye)
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.
Flags: needinfo?(gaye)
Looks good to me! r=me
(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 :)
Created attachment 8375341 [details] [review]
patch

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

Please help me review this again !! Thanks ;)
Attachment #8356047 - Attachment is obsolete: true
Attachment #8356047 - Flags: review?(evanxd)
Attachment #8375341 - Flags: review?(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.
Attachment #8375341 - Flags: review?(evanxd)
Hi EJ,

Just the last little nit to fix, then we could r+ to land. :)
Comment on attachment 8375341 [details] [review]
patch

Evan, we are almost there !!!
Attachment #8375341 - Flags: review?(evanxd)
Comment on attachment 8375341 [details] [review]
patch

Hi EJ,

Nice job! :D
Attachment #8375341 - Flags: review?(evanxd) → review+
master: 0d10f0e2f5aad393fa4baa1bc72cad3e673600be
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 956642

Updated

9 months ago
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.