Ubuntu Clock App

Merge lp:~brendan-donegan/ubuntu-clock-app/disable_location_prompt into lp:ubuntu-clock-app

  1. disable_location_prompt
  2. Merge into trunk
Proposed by Brendan Donegan on 2014-11-24
Status: Merged
Approved by: Nicholas Skaggs on 2015-05-27
Approved revision: 183
Merged at revision: 268
Proposed branch: lp:~brendan-donegan/ubuntu-clock-app/disable_location_prompt
Merge into: lp:ubuntu-clock-app
Diff against target: 70 lines (+46/-2)
2 files modified
tests/autopilot/ubuntu_clock_app/fixture_setup.py (+44/-0)
tests/autopilot/ubuntu_clock_app/tests/__init__.py (+2/-2)
To merge this branch: bzr merge lp:~brendan-donegan/ubuntu-clock-app/disable_location_prompt
Reviewer Review Type Date Requested Status
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve on 2015-05-27
Leo Arias 2014-11-24 Needs Fixing on 2014-11-27
Review via email: mp+242682@code.launchpad.net

Commit Message

Disable the location service prompt for testing purposes

Description of the Change

Disable the location service prompt for testing purposes

To post a comment you must log in.
Leo Arias (elopio) wrote :

6 +# Copyright 2014 Canonical

notice that this copyright header is missing a paragraph. You can copy the right header from test_alarm.py

17 +def set_location_service_testing(test_mode):

This would be nicer as a Fixture. And it should live in a testhelper module of the location service, not on the clock.

review: Needs Fixing
Brendan Donegan (brendan-donegan) wrote :

Yes I suppose it would be nicer as a fixture, I can do that easily. As for putting it in the location service, that's starting to get a little too involved for ops work. I could raise a bug for someone else to have a look.

Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :

FAILED: Continuous integration, rev:178
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~brendan-donegan/ubuntu-clock-app/disable_location_prompt/+merge/242682/+edit-commit-message

http://91.189.93.70:8080/job/ubuntu-clock-app-ci/587/
Executed test runs:

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/587/rebuild

review: Needs Fixing (continuous-integration)
Leo Arias (elopio) wrote :

<brendand> elopio, did you see my comment on the location prompt merge?
<brendand> elopio, i can make it a fixture but i can't invest the time to put it in location service
<brendand> elopio, since that's the rabbit hole of cmake and packaging
<brendand> elopio, i did that once for url-dispatcher and i'd rather chew off my arm than do it again :)
<elopio> brendand: that's the work man. I'm ok with you filing a bug and putting a comment on your code that it needs to be moved.
<elopio> but we need a way to give that bug a high priority.
<brendand> elopio, okay but who's going to address the bug?
<elopio> it's going to be needed by the scopes tests too.
<brendand> elopio, so for now make it a fixture and it can land?
<elopio> the clock, the weather, the rss, the camera, many webapps will be location-aware.
<elopio> brendand: yes, but lets agree that the next time it's needed, we won't copy it around.
<elopio> we'll find a place for it, and the only way for getting managers to assign some time for it is to leave the test failing.
<elopio> brendand: the url dispatcher is a case of success. We simplified the tests in dialer, messaging, unity and UX thanks to that one. Chewing your arm is not as good :)
<brendand> elopio, we don't have to land it right now if you think it will expidite the 'proper' fix
<brendand> elopio, heh - you didn't have to do it !
<elopio> actually, I think the dialer is not yet updated.
<elopio> brendand: no, lets land it. Please file the bug. Then next time we hit it, we will have the code working, and good evidence of why it needs to be moved.
<elopio> brendand: I know! You did it and I appreciate that. It should have been ted. That's the kind of thing we are documenting this week, an in theory all the managers have agreed to implement test helpers in their projects.
<elopio> brendand: in your bug, please mention that when the fixture is moved it will need a test.
<elopio> something like a simple qml app that only requests location, to check when the fixture is enabled and disabled.
<elopio> brendand: oh, and one more detail. Please don't use print. Use the logger.
<brendand> elopio, sure
<balloons> brendand, jenkins is running for your stuff now
<balloons> should be automagic
<elopio> brendand: well, two details. Also please be consistent using the single and double quotes for strings.
<elopio> for consistency with the rest of projects, use '

Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :

FAILED: Continuous integration, rev:178
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~brendan-donegan/ubuntu-clock-app/disable_location_prompt/+merge/242682/+edit-commit-message

http://91.189.93.70:8080/job/ubuntu-clock-app-ci/586/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-vivid/363/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/586/rebuild

review: Needs Fixing (continuous-integration)
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Brendan Donegan (brendan-donegan) wrote :

Done: https://bugs.launchpad.net/ubuntu/+source/trust-store/+bug/1397008

Jenkins tests passed, please top approve this

> <brendand> elopio, did you see my comment on the location prompt merge?
> <brendand> elopio, i can make it a fixture but i can't invest the time to put
> it in location service
> <brendand> elopio, since that's the rabbit hole of cmake and packaging
> <brendand> elopio, i did that once for url-dispatcher and i'd rather chew off
> my arm than do it again :)
> <elopio> brendand: that's the work man. I'm ok with you filing a bug and
> putting a comment on your code that it needs to be moved.
> <elopio> but we need a way to give that bug a high priority.
> <brendand> elopio, okay but who's going to address the bug?
> <elopio> it's going to be needed by the scopes tests too.
> <brendand> elopio, so for now make it a fixture and it can land?
> <elopio> the clock, the weather, the rss, the camera, many webapps will be
> location-aware.
> <elopio> brendand: yes, but lets agree that the next time it's needed, we
> won't copy it around.
> <elopio> we'll find a place for it, and the only way for getting managers to
> assign some time for it is to leave the test failing.
> <elopio> brendand: the url dispatcher is a case of success. We simplified the
> tests in dialer, messaging, unity and UX thanks to that one. Chewing your arm
> is not as good :)
> <brendand> elopio, we don't have to land it right now if you think it will
> expidite the 'proper' fix
> <brendand> elopio, heh - you didn't have to do it !
> <elopio> actually, I think the dialer is not yet updated.
> <elopio> brendand: no, lets land it. Please file the bug. Then next time we
> hit it, we will have the code working, and good evidence of why it needs to be
> moved.
> <elopio> brendand: I know! You did it and I appreciate that. It should have
> been ted. That's the kind of thing we are documenting this week, an in theory
> all the managers have agreed to implement test helpers in their projects.
> <elopio> brendand: in your bug, please mention that when the fixture is moved
> it will need a test.
> <elopio> something like a simple qml app that only requests location, to check
> when the fixture is enabled and disabled.
> <elopio> brendand: oh, and one more detail. Please don't use print. Use the
> logger.
> <brendand> elopio, sure
> <balloons> brendand, jenkins is running for your stuff now
> <balloons> should be automagic
> <elopio> brendand: well, two details. Also please be consistent using the
> single and double quotes for strings.
> <elopio> for consistency with the rest of projects, use '

Leo Arias (elopio) wrote :

If location service is not installed, the fixture shouldn't be called.
The fixture should be called only when needed. And as it is needed, it shouldn't hide the errors. It should fail the tests. Otherwise they will fail later with a cryptic message because the preconditions are not as expected.

review: Needs Fixing
Leo Arias (elopio) wrote :

Also, it feels wrong to use sudo during the test. Another ugly workaround we have to use because the location-service and the trust prompt where not consulted with QA, and there is no good test strategy around them. My suggestion is to involve jfunk and thomi in the discussion, so they put pressure on the devs to give this problem a high priority. Depending on what they say, it would be easier to decide if we should apply this workaround or wait for a proper fix.

Nicholas Skaggs (nskaggs) wrote :

Was a bug ever filed for any of this?

Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Alan Pope 🍺🐧🐱 🦄 (popey) wrote :

We need to revive this to get all our autopilot tests green. Going to change the status to get it back on our radar.

Alan Pope 🍺🐧🐱 🦄 (popey) wrote :

FYI:-

15:12 < iahmad> tvoss, this is still waiting to merge
                https://code.launchpad.net/~thomas-voss/trust-store/fix-1420790/+merge/249320
15:12 < tvoss> iahmad, yup, haven't come to it
15:12 < iahmad> popey, ^ tvoss branch should fix it in more official way

I Ahmad (iahmad) wrote :

Just to be clear: tvoss branch won't fix it on its own rather it will enable to write AP helpers to automate the trust-prompt, so more work would be needed on trust-store as well as on ubuntu-clock-app side.

Brendan Donegan (brendan-donegan) wrote :

We've been waiting too long for this - lets land it and in the event proper helpers are written for trust-prompt we can use those instead.

Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'tests/autopilot/ubuntu_clock_app/fixture_setup.py'
2--- tests/autopilot/ubuntu_clock_app/fixture_setup.py 1970-01-01 00:00:00 +0000
3+++ tests/autopilot/ubuntu_clock_app/fixture_setup.py 2014-11-27 12:27:57 +0000
4@@ -0,0 +1,44 @@
5+# Copyright (C) 2014 Canonical Ltd
6+#
7+# This file is part of Ubuntu Clock App
8+#
9+# Ubuntu Clock App is free software: you can redistribute it and/or modify
10+# it under the terms of the GNU General Public License version 3 as
11+# published by the Free Software Foundation.
12+#
13+# Ubuntu Clock App is distributed in the hope that it will be useful,
14+# but WITHOUT ANY WARRANTY; without even the implied warranty of
15+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
16+# GNU General Public License for more details.
17+#
18+# You should have received a copy of the GNU General Public License
19+# along with this program. If not, see <http://www.gnu.org/licenses/>.
20+
21+"""Clock app autopilot fixtures."""
22+
23+import fixtures
24+import logging
25+import subprocess
26+
27+
28+class LocationServiceTestEnvironment(fixtures.Fixture):
29+
30+ def setUp(self):
31+ super(LocationServiceTestEnvironment, self).setUp()
32+ self._set_location_service_testing(True)
33+ self.addCleanup(self._set_location_service_testing, False)
34+
35+ def _set_location_service_testing(self, test_mode):
36+ test = 'true' if test_mode else 'false'
37+ try:
38+ subprocess.check_call(
39+ 'sudo setprop custom.location.testing {}'.format(test),
40+ shell=True)
41+ subprocess.check_call(
42+ 'sudo restart ubuntu-location-service && '
43+ 'restart ubuntu-location-service-trust-stored',
44+ shell=True)
45+ except subprocess.CalledProcessError:
46+ logger = logging.getLogger(__name__)
47+ logger.error('Unable to start location service in testing mode '
48+ 'tests may fail as a result.')
49
50=== modified file 'tests/autopilot/ubuntu_clock_app/tests/__init__.py'
51--- tests/autopilot/ubuntu_clock_app/tests/__init__.py 2014-08-20 22:51:45 +0000
52+++ tests/autopilot/ubuntu_clock_app/tests/__init__.py 2014-11-27 12:27:57 +0000
53@@ -29,7 +29,7 @@
54 emulators as toolkit_emulators
55 )
56
57-from ubuntu_clock_app import emulators
58+from ubuntu_clock_app import emulators, fixture_setup
59
60 logger = logging.getLogger(__name__)
61
62@@ -60,7 +60,7 @@
63 # backup and wipe db's before testing
64 self.temp_move_sqlite_db()
65 self.addCleanup(self.restore_sqlite_db)
66-
67+ self.useFixture(fixture_setup.LocationServiceTestEnvironment())
68 launch, self.test_type = self.get_launcher_method_and_type()
69 self.useFixture(fixtures.EnvironmentVariable('LC_ALL', newvalue='C'))
70 super(ClockAppTestCase, self).setUp()

Subscribers

People subscribed via source and target branches