Closed Bug 1134257 Opened 5 years ago Closed 4 years ago

test_ftu_skip_tour should reflect new Send Data initial state and separate SIM- and network-available conditions

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: onelson, Assigned: gmealer)

References

Details

(Whiteboard: [3.0-Daily-Testing])

Attachments

(2 files, 1 obsolete file)

Attached file feb18_ftu.html
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
QA Whiteboard: [QAnalyst-Triage?][fxosqa-auto-backlog?]
Flags: needinfo?(pbylenga)
Whiteboard: [3.0-Daily-Testing]
NI on Parul, was this an expected change to have the "Yes, Send Data" unchecked in the FTU?
QA Whiteboard: [QAnalyst-Triage?][fxosqa-auto-backlog?] → [QAnalyst-Triage+][fxosqa-auto-backlog?]
Flags: needinfo?(pbylenga) → needinfo?(pmathur)
The "Yes, Send Data" has been unchecked by default in the FTU since Oct 2014 - see bug 1080827.
Flags: needinfo?(pmathur)
Parul, sorry, it is now checked instead of unchecked.
Flags: needinfo?(pmathur)
Depends on: 1134445
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.
Assignee: nobody → gmealer
Per https://bugzilla.mozilla.org/show_bug.cgi?id=1134445#c6 it sounds like this is intended (if not well-communicated) behavior now.
Summary: test_ftu_skip_tour failing consistently on 'About Firefox OS, Send Data' Check; checkbox default value changed from false to true → test_ftu_skip_tour should reflect new Send Data initial state and separate SIM- and network-available conditions
QA Whiteboard: [QAnalyst-Triage+][fxosqa-auto-backlog?] → [QAnalyst-Triage+][fxosqa-auto-sprint-10]
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.
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.
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.
Attachment #8566916 - Flags: review?(jlorenzo) → review-
(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.
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.
Attachment #8566916 - Flags: review- → review?(jlorenzo)
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.
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.
Attachment #8566916 - Flags: review?(jlorenzo) → review+
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.
Comment on attachment 8566916 [details] [review]
[gaia] geoelectric:bug-1134257 > mozilla-b2g:master

r+ from me. Looks good.
Attachment #8566916 - Flags: review?(jdorlus) → review+
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.
NI'ing myself for the update
Flags: needinfo?(gmealer)
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.
Flags: needinfo?(gmealer)
(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?
Flags: needinfo?(gmealer)
(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.
Flags: needinfo?(gmealer)
Ah, I see. I guess you need to use self.environment.phone_numbers now, instead of bool(self.testvars['local_phone_numbers']).
Comment on attachment 8566916 [details] [review]
[gaia] geoelectric:bug-1134257 > mozilla-b2g:master

Actually did turn out to be obsolete.
Attachment #8566916 - Attachment is obsolete: true
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.
Attachment #8621904 - Flags: review?(martijn.martijn)
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.
Attachment #8621904 - Flags: review?(martijn.martijn) → review+
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!
Depends on: 1172343
(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.
Flags: needinfo?(martijn.martijn)
(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.
Flags: needinfo?(martijn.martijn)
Merged: https://github.com/mozilla-b2g/gaia/commit/24032fbaee7fc3c2438ce6b674553e336ea8687e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
No longer depends on: 1172343
Depends on: 1175619
You need to log in before you can comment on or make changes to this bug.