Merge lp:~savilerow-team/savilerow/test-cleanup-and-py3 into lp:savilerow

[Chris Wayne]

Move to python3 so that phablet-test-run can be run

[Chris Wayne (cwayne)]

Tested by pushing these to /home/phablet/autopilot/custom then running 'phablet-test-run custom', and by ensuring they pass pep8 and pyflakes

[Chris Wayne (cwayne)]

cwayne@athena:~$ phablet-test-run custom
Loading tests from: /home/phablet/autopilot

Tests running...
No Custom Fonts Found

Ran 18 tests in 1.972s
OK

[Chris Wayne (cwayne)]

Also not that this MP does include some cleanup to the dconf test, which is frankly still less than ideal. However, the way this test is written must change imminently as the new dconf architecture is ready soon (since u-t-c-h has finally landed), so it doesn't make sense to put too much time fixing that test quite yet

[Scott Sweeny (ssweeny)]

> Also not that this MP does include some cleanup to the dconf test, which is
> frankly still less than ideal. However, the way this test is written must
> change imminently as the new dconf architecture is ready soon (since u-t-c-h
> has finally landed), so it doesn't make sense to put too much time fixing that
> test quite yet

I'll take the action to update the test along with the rest of the dconf stuff.

[Scott Sweeny (ssweeny)]

Update itself looks good.

review: Approve

[Alex Chiang (achiang)]

In general, not a fan of mixing a "port to new language" and "logic changes to test" in the same MP.

No justification for *why* dconf test no longer needs to test for comments.

Line 77, try..except block removed. Why? Old logic, the test would display every key that failed. New logic, test will terminate on first failure, and you won't learn about any future failures.

Lines 123--125, why intermediate variable needed? Why not just chain together the methods, such as:

data_dirs = subprocess.check_output(['initctl', 'get-env', 'XDG_DATA_DIRS']).decode().split(':')

Lines 146, 147 -- did some tool really recommend to split whitespace that way? Looks kinda ugly to me.

Lines 232, same question as 123.

Lines 302--305 -- why we no longer care about ValueError exception?

review: Needs Information

[Chris Wayne (cwayne)]

The dconf test wasn't actually using is_commented which made it clear to me that it wasn't necessary in the first place, and was just cluttering up the code.

Line 77, it would print out only information on one key, and then bail out of the test by doing self.assertTrue(False) so in that sense, the only thing that's changed is the lack of printing (which frankly I don't find that useful as the failed assertTrue will show you what is failing anyway)

TBH Line 123-125 was done that way just to stay under 80 chars to make pep8 not complain

146-147 again just trying to stay under 80 chars, couldn't figure out a better way to do it..

As for the ValueError, deleting that is an error on my part (was a little overzealous cleaning stuff up), will add it back in

[lp:~savilerow-team/savilerow/test-cleanup-and-py3]

9. By Chris Wayne on 2014-04-08

Shouldnt have removed valueerror, as that would indicate something wrong with the buildstamp and presumably the build itself

[Alex Chiang (achiang)]

On Tue, Apr 8, 2014 at 11:46 AM, Chris Wayne <email address hidden> wrote:
> The dconf test wasn't actually using is_commented which made it clear to me that it wasn't necessary in the first place, and was just cluttering up the code.

Well, the code was broken. because line 46 was is_comment and line 50
it's is_commented.

But the intention of the code doesn't seem wrong to me...

What does happen if you get comments in a dconf file now? In fact, we
should add a fixture that has comments in it.

[save that for a future MP that fixes the overall dconf test as
originally mentioned...]

>
> Line 77, it would print out only information on one key, and then bail out of the test by doing self.assertTrue(False) so in that sense, the only thing that's changed is the lack of printing (which frankly I don't find that useful as the failed assertTrue will show you what is failing anyway)

Fair enough -- I read that wrong because I missed the assertTrue.

But I still think it would be more useful to try..except, and continue
in the exception case, so that the test shows all the broken keys at
once.

And that should be another test fixture -- a dconf test file with 2
broken keys, again saved for the future MP.

>
> TBH Line 123-125 was done that way just to stay under 80 chars to make pep8 not complain
>
> 146-147 again just trying to stay under 80 chars, couldn't figure out a better way to do it..

Frankly I'd just ignore pep8's advice for those particular lines
because they make the code more annoying to read.

> As for the ValueError, deleting that is an error on my part (was a little overzealous cleaning stuff up), will add it back in

Thanks.

[lp:~savilerow-team/savilerow/test-cleanup-and-py3]

10. By Chris Wayne on 2014-04-08

Following pep8 here really makes it more difficult to read the code

[Chris Wayne (cwayne)]

So we do already ship comments in the dconf file today, and they're properly ignored, as we use re.match, the string will never match the given regex to find a key.

I do agree with the fact that getting a full list of failed keys will be useful, and plan to add it to a new MP.

[Alex Chiang (achiang)]

Looking better.

Few final nits:

- Line 45 comment should be removed, no?
- what changed on lines 86/87?
- 144/145 still looks worse than original 143
- 176/177 - what changed?
- 194/195 - why remove the trailing '.'?
- 303 - now I know for sure we are not testing that code path ;) syntax error in print statement. need to write a test that covers that path.

review: Needs Fixing

[lp:~savilerow-team/savilerow/test-cleanup-and-py3]

11. By Chris Wayne on 2014-04-08

Addressing MR comments

[Chris Wayne (cwayne)]

45 can be removed, but I thought it was referring to the test as a whole...

86/87 was trailing whitespace being removed (more pep8-ness)

176/177 more trailing whitespace

194/195 that . was the 80th char and I wanted clean pep8

Addressed 144/145 and 303

[Alex Chiang (achiang)]

New line numbers... 184/185 is user-facing documentation, so removing the trailing '.' is not acceptable.

Remember -- pep8 is only a set of guidelines, not a hard rule. The rules can be bent when it makes sense to do so.

review: Needs Fixing

[lp:~savilerow-team/savilerow/test-cleanup-and-py3]

12. By Chris Wayne on 2014-04-08

Addressing MR comments

[Chris Wayne (cwayne)]

Updated as per MR comments

[Alex Chiang (achiang)]

Thanks.

Looking forward to the dconf cleanup MP. :)

review: Approve

=== modified file 'tests/api/test_clickapps.py'
--- tests/api/test_clickapps.py	2014-03-06 16:14:03 +0000
+++ tests/api/test_clickapps.py	2014-04-08 20:11:31 +0000
@@ -5,6 +5,7 @@
 #: Custom click app installation directory
 path = "/custom/click"
 
+
 class ClickAppVerificationTests(AutopilotTestCase):
     """
     Downstream may ship pre-installed click packages as part of the
@@ -34,7 +35,7 @@
         against the file list in the target directory.
         """
         # Get the list of installed click apps
-        installed_apps = subprocess.check_output(['click', 'list'])
+        installed_apps = subprocess.check_output(['click', 'list']).decode()
 
         apps = os.listdir(path)
         for a in apps:

=== modified file 'tests/api/test_dconf.py'
--- tests/api/test_dconf.py	2014-03-06 16:14:03 +0000
+++ tests/api/test_dconf.py	2014-04-08 20:11:31 +0000
@@ -6,6 +6,7 @@
 #: Path to pre-seeded custom dconf database
 custom_dconf_key = '/custom/etc/dconf/db/custom.d/custom.key'
 
+
 class DconfVerificationTests(AutopilotTestCase):
 
     os.environ['DCONF_DIR'] = '/custom/etc/dconf'
@@ -23,36 +24,23 @@
         try:
             ck = open(custom_dconf_key, 'r')
         except IOError:
-            print "File not found, has this image not been customized?"
-            self.asserttrue(False)
+            print("File not found, has this image not been customized?")
+            self.assertTrue(False)
 
         dconf_keys = {}
 
         for line in ck.readlines():
             # XXX: This seems so broken.
-            is_comment = False
-            if re.match('\#', line.lstrip()):
-                # TODO turn to debug statements
-                print 'KEY/VALUE IS COMMENTED SO NOT USED, SKIPPING'
-                is_commented = True
-            if re.match('\[.*\]', line) and not is_comment:
+            if re.match('\[.*\]', line):
                 keybase = re.sub('[\[\]]', '', line)
                 keybase = '/%s/' % (keybase.strip('\n'))
-            if re.match('.*\=.*', line) and not is_comment:
+            if re.match('.*\=.*', line):
                 key = keybase + line.split('=')[0]
                 value = line.split('=')[1]
                 dconf_keys[key] = value
 
-        print dconf_keys
-
-        for key, value in dconf_keys.iteritems():
-            # Properly installed dconf keys should be visible via 
+        for key, value in dconf_keys.items():
+            # Properly installed dconf keys should be visible via
             # `dconf read`
-            dval = subprocess.check_output(['dconf', 'read', key])
-            try:
-                self.assertEqual(dval, value)
-            except AssertionError:
-                print ""
-                print "ERROR"
-                print "%s does not match. FAILING" % key
-                self.asserttrue(False)
+            dval = subprocess.check_output(['dconf', 'read', key]).decode()
+            self.assertEqual(dval, value)

=== modified file 'tests/api/test_fonts.py'
--- tests/api/test_fonts.py	2014-03-06 16:14:03 +0000
+++ tests/api/test_fonts.py	2014-04-08 20:11:31 +0000
@@ -2,7 +2,7 @@
 import subprocess
 import os
 
-#: Path where pre-installed custom fonts are installed 
+#: Path where pre-installed custom fonts are installed
 fontspath = "/custom/usr/share/fonts"
 fontlist = []
 
@@ -23,11 +23,11 @@
                     fontlist.append(os.path.join(path, name))
 
             # Properly installed fonts are visible to fc-list
-            installed_fonts = subprocess.check_output(['fc-list'])
+            installed_fonts = subprocess.check_output(['fc-list']).decode()
 
             # Compare filesystem list against fc-list output
             for font in fontlist:
                 self.assertIn(font, installed_fonts)
         else:
-            print "No Custom Fonts Found"
+            print("No Custom Fonts Found")
             self.assertTrue(True)

=== modified file 'tests/api/test_infographic_themes.py'
--- tests/api/test_infographic_themes.py	2014-03-06 16:14:03 +0000
+++ tests/api/test_infographic_themes.py	2014-04-08 20:11:31 +0000
@@ -5,6 +5,7 @@
 #: Relative path to welcome screen infographic customization file
 themefile = 'libusermetrics/themes/custom.xml'
 
+
 class InfographicThemeCustomizationTest(AutopilotTestCase):
 
     def test_theme_dir_in_datadirs(self):
@@ -16,8 +17,7 @@
         in the system.
         """
         themefilefound = False
-        data_dirs = subprocess.check_output(['initctl', 'get-env',
-                                             'XDG_DATA_DIRS']).split(':')
+        data_dirs = subprocess.check_output(['initctl', 'get-env','XDG_DATA_DIRS']).decode().split(":")
         for dir in data_dirs:
             # We use a relative path in `themefile` because we don't
             # necessarily know the full path to the custom XDG data

=== modified file 'tests/api/test_lockscreen.py'
--- tests/api/test_lockscreen.py	2014-03-06 16:14:03 +0000
+++ tests/api/test_lockscreen.py	2014-04-08 20:11:31 +0000
@@ -6,6 +6,7 @@
 #: Path to custom welcome screen graphic
 lockscreenpath = "/custom/usr/share/backgrounds/welcome.png"
 
+
 class LockScreenCustomizationTests(AutopilotTestCase):
     def test_lockscreen(self):
         """

=== modified file 'tests/api/test_scopes.py'
--- tests/api/test_scopes.py	2014-03-29 21:10:56 +0000
+++ tests/api/test_scopes.py	2014-04-08 20:11:31 +0000
@@ -1,14 +1,15 @@
 from autopilot.testcase import AutopilotTestCase
 import os
 import psutil
-import subprocess
 
 #: List of custom scopes that should be running
-scopes = ["scope-tourism", "scope-tourism-mocknews", "scope-tourism-mocktransport",
-          "scope-tourism-mockrecommends", "scope-tourism-mockweather"]
+scopes = ["scope-tourism", "scope-tourism-mocknews",
+          "scope-tourism-mocktransport", "scope-tourism-mockrecommends",
+          "scope-tourism-mockweather"]
 #: Custom scope installation directory
 path = "/custom/lib/arm-linux-gnueabihf/unity-scopes"
 
+
 class ScopeVerificationTest(AutopilotTestCase):
     """
     Downstream may ship pre-installed customized scopes as part of the
@@ -29,7 +30,7 @@
     def test_scope_customization_running_process(self):
         """
         Insalled custom scopes should be running processes, started by
-        scoperunner.  This test ensures that the scopes are properly 
+        scoperunner.  This test ensures that the scopes are properly
         started
         """
         proc_cmd_list = []
@@ -38,4 +39,3 @@
             proc_cmd_list.append(proc_info)
         for scope in scopes:
             self.assertTrue(any(scope in proc for proc in proc_cmd_list))
-

=== modified file 'tests/api/test_themes.py'
--- tests/api/test_themes.py	2014-03-06 16:14:03 +0000
+++ tests/api/test_themes.py	2014-04-08 20:11:31 +0000
@@ -8,6 +8,7 @@
 themedir = 'themes/Ubuntu/Custom'
 themestring = "theme=themes.Ubuntu.Custom"
 
+
 class ThemeCustomizationTests(AutopilotTestCase):
     """
     Downstreams may customize the visual color scheme of Ubuntu Touch,
@@ -17,7 +18,7 @@
 
       * create the elements of a new visual theme
       * configure the Unity shell to use the custom theme
-    
+
     Downstreams *may* create new visual themes.
 
     If a new visual theme is created, downstreams *must* specify the new
@@ -42,10 +43,9 @@
         if "%s" % (themestring) in customk.splitlines():
             self.assertTrue(True)
         else:
-            print "Error reading theme.ini at %s" % (custom_theme)
+            print("Error reading theme.ini at %s" % (custom_theme))
             self.assertTrue(False)
 
-
     def test_theme_dir_in_datadirs(self):
         """
         If a downstream creates a custom visual theme, the theme's files
@@ -60,8 +60,7 @@
         the specific subdirectory containing the custome theme files.
         """
         themedirfound = False
-        data_dirs = subprocess.check_output(['initctl', 'get-env',
-                                             'XDG_DATA_DIRS']).split(':')
+        data_dirs = subprocess.check_output(['initctl', 'get-env','XDG_DATA_DIRS']).decode().split(":")
         for dir in data_dirs:
             # We use a relative path in `themedir` because we don't
             # necessarily know the full path to the custom XDG data

=== modified file 'tests/api/test_webbrowser_settings.py'
--- tests/api/test_webbrowser_settings.py	2014-03-06 16:14:03 +0000
+++ tests/api/test_webbrowser_settings.py	2014-04-08 20:11:31 +0000
@@ -1,7 +1,6 @@
 from autopilot.testcase import AutopilotTestCase
 import os
 import sqlite3
-import subprocess
 
 #: Pre-seeded browser bookmark database
 dbpath = os.path.expanduser("~/.local/share/webbrowser-app/bookmarks.sqlite")
@@ -9,6 +8,7 @@
 #: File specifying custom browser home page
 settingspath = os.path.expanduser("~/.config/webbrowser-app/settings.conf")
 
+
 class BrowserCustomizationTests(AutopilotTestCase):
     """
     Downstreams *may* modify the default web browser with custom
@@ -25,9 +25,6 @@
         """
         Check that the pre-seeded bookmark database is properly formed.
         """
-        # Quick ls -la and md5sum of the bookmarks.sqlite file for debug
-        print subprocess.check_output(['ls', '-al', dbpath])
-        print subprocess.check_output(['md5sum', dbpath])
         conn = sqlite3.connect(dbpath)
         conn.row_factory = sqlite3.Row
 
@@ -36,7 +33,7 @@
         r = c.fetchone()
 
         # Any and every row's keys should have these columns
-        schema = r.keys()
+        schema = list(r.keys())
         dbcolumns = ['url', 'title', 'icon']
         self.assertTrue(schema == dbcolumns)
 

=== modified file 'tests/core/test_build.py'
--- tests/core/test_build.py	2014-03-06 16:14:03 +0000
+++ tests/core/test_build.py	2014-04-08 20:11:31 +0000
@@ -1,9 +1,11 @@
 from autopilot.testcase import AutopilotTestCase
-import os, datetime
+import os
+import datetime
 
 #: Custom tarball build timestamp, generated by jenkins
 path = "/custom/build_id"
 
+
 class BuildCustomizationTests(AutopilotTestCase):
 
     def test_build_id(self):
@@ -27,12 +29,11 @@
             self.assertTrue(os.path.exists(path))
             f = open(path, 'r')
             build_id = f.read().strip()
-            print(build_id)
             self.assertTrue(datetime.date.fromtimestamp(float(build_id)))
             f.close()
         except IOError as e:
-            print "build_id file does not exist: %s" % e
+            print("build_id file does not exist: %s" % e)
             self.assertTrue(False)
         except ValueError as e:
-            print "problem with the format of the build_id file: %s" % e
+            print("problem with the format of the build_id file: %s" % e)
             self.assertTrue(False)

=== modified file 'tests/core/test_customization_enabled.py'
--- tests/core/test_customization_enabled.py	2014-03-06 16:14:03 +0000
+++ tests/core/test_customization_enabled.py	2014-04-08 20:11:31 +0000
@@ -4,6 +4,7 @@
 #: Flag to indicate pre-loaded sample content has been copied once
 customized_flag_path = os.path.expanduser("~/.customized")
 
+
 class GeneralCustomizationEnablement(AutopilotTestCase):
 
     def test_customization_enablement(self):
@@ -18,7 +19,7 @@
         The presence of the `~/.customized` flag signals that the sample
         content has already been copied to the user area, and that it
         should not be copied again.
-        
+
         This implementation allows the user to delete the sample
         content if desired, but will also properly re-copy the content
         to a new user after a 'factory reset' event.

=== modified file 'tests/core/test_upstartjobs.py'
--- tests/core/test_upstartjobs.py	2014-03-06 16:14:03 +0000
+++ tests/core/test_upstartjobs.py	2014-04-08 20:11:31 +0000
@@ -9,6 +9,7 @@
                '/custom/etc/dconf_profile', 'XDG_DATA_DIRS': '/custom/xdg/data'
                }
 
+
 class UpstartCustomizationTests(AutopilotTestCase):
     """
     To implement the Ubuntu Savvy architecture, the contents of the