Oliver Nelson [:oliverthor] Reporter
	Description • 9 years ago

Description:
test_ftu_skip_tour.py is failing 10/10 via local automation after failing in it's smoke run for Jenkins:
http://jenkins1.qa.scl3.mozilla.com/job/flame-kk-319.mozilla-central.nightly.ui.functional.smoke/37/
The check appears to toggle from 'unchecked', to 'checked', to 'unchecked' within a span of 2 seconds before the case hangs and times out.
The 'Yes! Send data.' checkbox appears to be setting to 'True' as it's default value within this nightly, as opposed to yesterdays.

Traceback (most recent call last):
TEST-START | test_ftu_skip_tour.py TestFtu.test_ftu_skip_tour
TEST-UNEXPECTED-FAIL | test_ftu_skip_tour.py TestFtu.test_ftu_skip_tour | AssertionError: False is not true

Traceback (most recent call last):
  File "/home/flash/Desktop/oliverthor/git/gaia/tests/python/gaia-ui-tests/.env/local/lib/python2.7/site-packages/marionette_client-0.8.7-py2.7.egg/marionette/marionette_test.py", line 283, in run
    testMethod()
  File "/home/flash/Desktop/oliverthor/git/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/ftu/test_ftu_skip_tour.py", line 106, in test_ftu_skip_tour
    self.assertTrue(self.data_layer.get_setting('debug.performance_data.shared'))
TEST-INFO took 96578ms

Environmental Variables:
Device firmware (base)	L1TC100118D0
Device firmware (date)	18 Feb 2015 01:45:09
Device firmware (incremental)	eng.cltbld.20150218.044458
Device firmware (release)	4.4.2
Device identifier	flame
Gaia date	17 Feb 2015 15:32:21
Gaia revision	82f286f10a41
Gecko build	20150218010226
Gecko revision	9696d1c4b3ba
Gecko version	38.0a1

Reproducible manually: YES

Repro frequency: 10/10

	Peter Bylenga [:PBylenga]
	Comment 1 • 9 years ago

NI on Parul, was this an expected change to have the "Yes, Send Data" unchecked in the FTU?

	Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead
	Comment 2 • 9 years ago

The "Yes, Send Data" has been unchecked by default in the FTU since Oct 2014 - see bug 1080827.

	Peter Bylenga [:PBylenga]
	Comment 3 • 9 years ago

Parul, sorry, it is now checked instead of unchecked.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 4 • 9 years ago

I've got this one. 

There's a secondary problem with this test that it assumes the cell data screen appears only if there's a valid mobile connection, which is untrue--appears even with an inactive sim in. This may be the source of some of the previous spurious failures when cell signals fluctuate. I'm changing to logic to check for a sim, whether active or not, for the purposes of paging through the wizard.

I'll have a pull request for both issues in tomorrow if that's ok. If anyone feels strongly I should separate them, let me know and I'll do so.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 5 • 9 years ago

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1134445#c6 it sounds like this is intended (if not well-communicated) behavior now.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 7 • 9 years ago

As mentioned above, aside from fixing the checkbox state (and making the verification more valid by making sure it will actually look for a change), I did put in an additional change in to separate concerns between having a SIM and having data. 

I needed to do this inline to debug against my phone, as right now the SIM isn't active, but it's also the proper behavior. I can split these if necessary, but it's a pain to do two PRs on the same file.

I opted to use testvars instead of API for the SIM-expected condition, since having a SIM is an externally-driven condition and we're looking with consistency with external state (i.e. black box), not just consistency with whatever the API reports back. The latter is more of an Integration concern.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 8 • 9 years ago

One other comment: 

I'm assuming the backend value for the default sharing state is accurate and verifying the FTU API against that, rather than hardcoding that it starts True. 

If the intention of the test is to specifically verify the default system value as well, this is incomplete. Let me know and I'll restore a hard-coded verification of the backend state too.

	Johan Lorenzo [:jlorenzo]
	Comment 9 • 9 years ago

Comment on attachment 8566916 [details] [review]
[gaia] geoelectric:bug-1134257 > mozilla-b2g:master

It looks way better than the code we had before this PR.

I'm r-'ing because, to me, we can't go with a dynamic value as an expected value for an assertion. We could miss bugs like bug 1134445.

A less important no-go is the semantic duplication of 2 functions: tap_statistics_checkbox and toggle_statistics. This would be a pain to understand why we have 2 functions which do the same thing, in the future.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 10 • 9 years ago

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #9)
> Comment on attachment 8566916 [details] [review]
> [gaia] geoelectric:bug-1134257 > mozilla-b2g:master
> 
> It looks way better than the code we had before this PR.
> 
> I'm r-'ing because, to me, we can't go with a dynamic value as an expected
> value for an assertion. We could miss bugs like bug 1134445.
> 
> A less important no-go is the semantic duplication of 2 functions:
> tap_statistics_checkbox and toggle_statistics. This would be a pain to
> understand why we have 2 functions which do the same thing, in the future.

Understood re: dynamic. FWIW, I did check the Moztrap testcase, and this is not part of the verification for that case. I think there's a case to be made for verifying default values a little differently (in particular, I think they should be data-driven, not embedded in test cases) but I'm happy to add it here for now.

Tap vs. Toggle is consistent with the Settings app and a number of others (I checked). Tap is a blind tap of the control. Toggle is a more semantic operation, with a wait that the value actually changed.

More to the point, I need the Toggle operation, and it should be in the app object. I can't remove the Tap since it's already established and other test code (possibly not ours) is using it.

I do agree that the split there is a little awkward but, as I said, I was shooting for consistency. When we put together best practices for future app/views, I have better ways to split this up (basics: there shouldn't be a tap wrapper function--instead app object should export stats_checkbox as a property, so you can stats_checkbox.tap(), with the app object otherwise only having semantic wrappers). 

I just can't implement them here because you can't move backwards on a published interface without a ton of risk.

So, takeaway:

I'll make change 1, but not change 2. I'll ask for re-review, but please take my justifications into account re: refusing change 2.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 11 • 9 years ago

Comment on attachment 8566916 [details] [review]
[gaia] geoelectric:bug-1134257 > mozilla-b2g:master

PR is updated. Echoing comment from Github:

Differences from last:

* I changed the expected start state of Share Data to a hardcoded True.
* In order to make that a complete verification, I'm now verifying three things:

1) UI matches hardcoded expected start state
2) The UI state is consistent with back end
3) Toggling the UI state toggled the back end

Together that verifies everything Johan was concerned with, and also establishes a before state (necessary to know that your after state actually changed anything).

* I changed the naming of toggle_ and is_*_enabled to share_data instead of statistics. 

That clearly differentiates between functions dealing with nuts and bolts of the specific checkbox and functions dealing with the more general concept of the UI state, and should make more clear why these aren't redundant.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 12 • 9 years ago

Bug 1134445 is starting to look less sure re: the defaults change. We may need to wait to land this until that's settled. Possible I should just put a skip in instead in the meantime.

The original PR would have accommodated either decision (part of the reason I did it that way, since the specific default wasn't part of the test scope being automated), but the hardcoded one not so much. 

FWIW, this is exactly why I wanted the logic around share data implemented around a single boolean control variable, though, rather than using separate enable/disable methods.

	Johan Lorenzo [:jlorenzo]
	Comment 13 • 9 years ago

Comment on attachment 8566916 [details] [review]
[gaia] geoelectric:bug-1134257 > mozilla-b2g:master

Okay for having both methods here. Also, r+ modulo bug 1134445 gets fixed.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 14 • 9 years ago

After reviewing Dave's https://bugzilla.mozilla.org/show_bug.cgi?id=1135721 patch, realized my check for local_phone_number is going to break if there's not one defined, so I need to slightly rework that part prior to checkin anyway.

	John Dorlus [:Silne30]
	Comment 15 • 9 years ago

Comment on attachment 8566916 [details] [review]
[gaia] geoelectric:bug-1134257 > mozilla-b2g:master

r+ from me. Looks good.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 16 • 9 years ago

OK, sounds like the blocker bug resolved in favor of this behavior. I need to check it out on a current build and possibly rebase, so expect a possible followup review.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 17 • 9 years ago

NI'ing myself for the update

	Oliver Nelson [:oliverthor] Reporter
	Comment 18 • 9 years ago

Bug 1134445 has been 'Resolved Fixed', confirming that the state of the "Send Data" checkbox to be true (state confirmed in https://bugzilla.mozilla.org/show_bug.cgi?id=1134445#c11) is intended behavior and resolved by the privacy team (https://bugzilla.mozilla.org/show_bug.cgi?id=1134445#c25).

This test should flip it's state of 'Expected-Fail' from encountering a 'true' checkbox initially to pass in this state.

	Martijn Wargers (dead)
	Comment 19 • 9 years ago

(In reply to Geo Mealer [:geo] from comment #16)
> OK, sounds like the blocker bug resolved in favor of this behavior. I need
> to check it out on a current build and possibly rebase, so expect a possible
> followup review.

Geo, this pull request still needs to land, right? Are you up to it, or do you want me to take the work?

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 20 • 9 years ago

(In reply to Martijn Wargers [:mwargers] (QA) from comment #19)
> 
> Geo, this pull request still needs to land, right? Are you up to it, or do
> you want me to take the work?

Thanks for the nudge! I've got it. I need to fix the issue in comment 14 and will then put the refresh up for review. Look for it tomorrow.

	Martijn Wargers (dead)
	Comment 21 • 9 years ago

Ah, I see. I guess you need to use self.environment.phone_numbers now, instead of bool(self.testvars['local_phone_numbers']).

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 22 • 9 years ago

Comment on attachment 8566916 [details] [review]
[gaia] geoelectric:bug-1134257 > mozilla-b2g:master

Actually did turn out to be obsolete.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 23 • 9 years ago

Updated pull request. The layout (and thus locator) for the geolocation box also changed while this was disabled. Looks like it's implemented with a custom control now.

I ran the FTU section of the manifest with no problems on-device.

	Martijn Wargers (dead)
	Comment 24 • 9 years ago

Comment on attachment 8621904 [details] [review]
[gaia] geoelectric:bug-1134257 > mozilla-b2g:master

Looks perfect!
I ran it 10 times on device locally without any failures.

	Martijn Wargers (dead)
	Comment 25 • 9 years ago

Ok, I now tried to repeat this test 30 times, but then I get the problem in bug 1172343.
So we should wait to check this in, probably, until bug 1172343 is resolved.
But it's actually nice that this happens, because it makes it easier to reproduce bug 1172343!

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07 Assignee
	Comment 26 • 9 years ago

(In reply to Martijn Wargers [:mwargers] (QA) from comment #25)
> Ok, I now tried to repeat this test 30 times, but then I get the problem in
> bug 1172343.
> So we should wait to check this in, probably, until bug 1172343 is resolved.
> But it's actually nice that this happens, because it makes it easier to
> reproduce bug 1172343!

Doesn't that bug repro across a number of tests? If that misbehavior isn't dependent on this PR, I'd like to check in before the PR bitrots.

	Martijn Wargers (dead)
	Comment 27 • 9 years ago

(In reply to Geo Mealer [:geo] from comment #26)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #25)
> Doesn't that bug repro across a number of tests? If that misbehavior isn't
> dependent on this PR, I'd like to check in before the PR bitrots.

I guess you're right, things are already quite bad anyway. It at least allowed me to create a minimal testcase that reproduces the issue.

	Martijn Wargers (dead)
	Comment 28 • 9 years ago

Merged: https://github.com/mozilla-b2g/gaia/commit/24032fbaee7fc3c2438ce6b674553e336ea8687e