1202388 - Factorize the checkboxes and the switches into 1 class

Status: RESOLVED FIXED

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

Description

[Johan Lorenzo [:jlorenzo]]

That was discussed in bug 1182522. It turns out the work has a bigger scope than the bug was implying. Let's put the preliminary work here.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

Comment 2

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

There are 2 commits in this PR: the first one is for the Gaia switches/checkboxes. The second one is for their HTML equivalents.

To find every single instance, I did:
1. In PyCharm, "Find usage" of is_custom_element_selected
2. git grep 'wrappedJSObject.checked'
3. git grep 'is_selected()'

As you can see, I changed the remaining toggle_. I'd say, this is the most-error prone part of the PR. I might have misunderstood if we needed to be enabled or not at some point. I started an adhoc job[1], to make sure I didn't break anything

[1] http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/883/

Comment 3

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #2)
> To find every single instance, I did:
And it turns out I forgot to search for 'expected.element_enabled', which hides 17 other places where we can factorize. I'll create a 3rd commit.

Comment 4

[Johan Lorenzo [:jlorenzo]]

It turns out to be longer than I expected. In the Settings app we use HTML elements but in a different way: the checkbox is in the DOM but is not visible to the user, you have to tap the label. I introduced this type of Binary component in a 3rd commit.

I cleaned out the 3 commits. You should be able to review 1 commit without taking a look at the others.

Before asking you guys on review, let's check I didn't break anything: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/884/console

Comment 5

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

The adhoc job showed some errors that are not related to the changes I made:
* The spark tests ran against a non-spark build
* The accessibility tests are failing, but I didn't change the a11y methods
* some punctual error related to other open bugs

One failure is related to the changes, though: test_clock_create_new_alarm.py. I tried to repro locally and I couldn't. I think I'm facing an intermittent failure. I started [1], to make sure of that. Until I find the cause of the failure, I think we're good for a first round of review.

[1] http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/885/console

Comment 6

[Johan Lorenzo [:jlorenzo]]

That was passing locally because I was not on the right branch. The test is permafailing on Jenkins. I'll look into it.

Comment 7

[Johan Lorenzo [:jlorenzo]]

I pushed a fix. Let's see how intermittent this test is: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/887/console

Comment 8

[Martijn Wargers (dead)]

Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

I really like this patch!
I won't be complaining about any naming any more.

I'm just wondering, I see lots of stuff like this:
+    def enable_data_alert(self):
-        self.marionette.find_element(*self._data_alert_switch_locator).tap()
+        self._data_alert_switch.enable()

And then in the test you use:
ftu_step3.enable_data_alert()

Wouldn't it be better to just use self.data_alert_switch.enable() in the test?

Comment 9

[Martijn Wargers (dead)]

Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

It looks great!
There is one issue with InvisibleHtmlBinaryControl you need to fix, it doesn't have an is_checked property.
For the rest only a few nits.

I wonder if GaiaBinaryControl/HtmlBinaryControl/InvisibleHtmlBinaryControl
could just be combined into a BinaryControl.
Then we could just use that all over the place and when something would then be switched over from input type=checkbox to gaia-switch, no tests of ours would get broken.
But I guess that's impossible because of InvisibleHtmlBinaryControl which needs an extra locator to tap on the label.

Comment 10

[No-Jun Park [:njpark]]

Comment on attachment 8657783 [details] [review]
[gaia] JohanLorenzo:bug-1202388 > mozilla-b2g:master

I like this change too, it might break some of the methods in my current PR, but it should be easy to remedy.  Let's put it in and see what happens.

Comment 11

[No-Jun Park [:njpark]]

Johan, I noticed that immediately after the switch is toggled, the switch is temporarily greyed out, and its attribute named 'disabled' is set to true.  When in this state, it's visible, but tapping it won't do anything.  

So I do a check like the following:
Wait(self.marionette).until(lambda m: self.marionette.find_element(
*self._cell_data_enabled_input_locator).get_attribute('disabled') == 'false')

Perhaps if this is universal, we could incorporate this into your PR?

Comment 12

[Johan Lorenzo [:jlorenzo]]

Before the merge, I renamed the 'FormControl' class to 'Widget'

(In reply to Martijn Wargers [:mwargers] (QA) from comment #8) 
> Wouldn't it be better to just use self.data_alert_switch.enable() in the
> test?
A more detailed comment in Github[1], in order to not break the law of Demeter and for consistency, it's better to no use self._a_switch.enable directly in a test.


(In reply to Martijn Wargers [:mwargers] (QA) from comment #9)
> There is one issue with InvisibleHtmlBinaryControl you need to fix, it
> doesn't have an is_checked property.
The is_checked property is inherited from HtmlBinaryControl. No changes to do.

> For the rest only a few nits.
Fixed. Details in the PR.

> I wonder if GaiaBinaryControl/HtmlBinaryControl/InvisibleHtmlBinaryControl
> could just be combined into a BinaryControl.
I'm not sure how we could effectively know if we have to tap it "the gaia way" or the other. Worst case scenario, if one panel changes its switches, we have 1 line to change for each switch.


(In reply to No-Jun Park [:njpark] from comment #11)
> So I do a check like the following:
> Wait(self.marionette).until(lambda m: self.marionette.find_element(
> *self._cell_data_enabled_input_locator).get_attribute('disabled') == 'false')
Like discussed on IRC, I'm not 100% sure that el.get_attribute('disabled') and el.is_enabled are exactly the same behavior. They don't call the same marionette method, but if so, we do the wait at [2].


[1] https://github.com/mozilla-b2g/gaia/pull/31718#discussion_r39138742
[2] https://github.com/mozilla-b2g/gaia/pull/31718/files#diff-39a884f07bb9900df9e98c34ae425e20R30

Comment 13

[Martijn Wargers (dead)]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #12)
> Like discussed on IRC, I'm not 100% sure that el.get_attribute('disabled')
> and el.is_enabled are exactly the same behavior. They don't call the same
> marionette method, but if so, we do the wait at [2].

I think you're right. I guess this doesn't work for custom elements.
If Marionette does something like this (and there is indication it does):
https://code.google.com/p/selenium/source/browse/javascript/atoms/dom.js?r=d5fbbd43adecb829134167017c66722dcc06b491#318
Then is_enabled only works for certain form elements.

Comment 14

[Johan Lorenzo [:jlorenzo]]

Landed in master at: https://github.com/mozilla-b2g/gaia/commit/e1fc5d6a83925ed4fac547a7fe4bacd3dd44ede2