Closed
Bug 1134257
Opened 10 years ago
Closed 9 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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: onelson, Assigned: gmealer)
References
Details
(Whiteboard: [3.0-Daily-Testing])
Attachments
(2 files, 1 obsolete file)
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
Reporter | ||
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?][fxosqa-auto-backlog?]
Flags: needinfo?(pbylenga)
Whiteboard: [3.0-Daily-Testing]
Comment 1•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
Parul, sorry, it is now checked instead of unchecked.
Flags: needinfo?(pmathur)
Assignee | ||
Comment 4•10 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.
Assignee: nobody → gmealer
Flags: needinfo?(pmathur)
Assignee | ||
Comment 5•10 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.
Assignee | ||
Updated•10 years ago
|
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
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8566916 -
Flags: review?(jlorenzo)
Assignee | ||
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage+][fxosqa-auto-backlog?] → [QAnalyst-Triage+][fxosqa-auto-sprint-10]
Assignee | ||
Updated•10 years ago
|
Attachment #8566916 -
Flags: review?(jdorlus)
Assignee | ||
Comment 7•10 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.
Assignee | ||
Comment 8•10 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.
Comment 9•10 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.
Attachment #8566916 -
Flags: review?(jlorenzo) → review-
Assignee | ||
Comment 10•10 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.
Assignee | ||
Comment 11•10 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.
Attachment #8566916 -
Flags: review- → review?(jlorenzo)
Assignee | ||
Comment 12•10 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.
Comment 13•10 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.
Attachment #8566916 -
Flags: review?(jlorenzo) → review+
Assignee | ||
Comment 14•10 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.
Comment 15•9 years ago
|
||
Comment on attachment 8566916 [details] [review] [gaia] geoelectric:bug-1134257 > mozilla-b2g:master r+ from me. Looks good.
Attachment #8566916 -
Flags: review?(jdorlus) → review+
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.
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.
Updated•9 years ago
|
Flags: needinfo?(gmealer)
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?
Flags: needinfo?(gmealer)
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.
Flags: needinfo?(gmealer)
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']).
Assignee | ||
Updated•9 years ago
|
Attachment #8566916 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8566916 -
Attachment is obsolete: false
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.
Attachment #8566916 -
Attachment is obsolete: true
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.
Attachment #8621904 -
Flags: review?(martijn.martijn)
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.
Attachment #8621904 -
Flags: review?(martijn.martijn) → review+
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!
Depends on: 1172343
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.
Flags: needinfo?(martijn.martijn)
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.
Flags: needinfo?(martijn.martijn)
Comment 28•9 years ago
|
||
Merged: https://github.com/mozilla-b2g/gaia/commit/24032fbaee7fc3c2438ce6b674553e336ea8687e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•