Merge lp:~jdstrand/click-reviewers-tools/lp1346481 into lp:click-reviewers-tools

[Jamie Strandboge]

add push-helper checks:
  - add bin/click-check-push-helper
  - add clickreviews/cr_push_helper.py
  - add clickreviews/tests/test_cr_push_helper.py
  - update bin/click-show-files
  - update bin/click-run-checks
  - update clickreviews/cr_tests.py

  clickreviews/cr_desktop.py:
  - skip missing desktop hook with push-helper
  - comment out noisy msg()

  clickreviews/cr_lint.py:
  - add push-helper as known hook
  - allow multiple apps in click, but not multiple desktop apps
  - clarify check_hooks() error

  clickreviews/tests/test_cr_lint.py: adjust for above

  clickreviews/cr_security.py:
  - add template tests for push-helper
  - add policy_groups tests for push-helper
  - clarify check_policy_groups() output

  clickreviews/tests/test_cr_security.py: adjust for above

  clickreviews/cr_common.py: fix pep8 error from merge from trunk

[lp:~jdstrand/click-reviewers-tools/lp1346481]

226. By Jamie Strandboge on 2014-07-24

merge with trunk:
[ Daniel Holbach ]
refer to documentation about click in case we encounter .deb packages.

227. By Jamie Strandboge on 2014-07-24

clickreviews/cr_common.py: fix pep8 error

[John Lenton (chipaca)]

I haven't been able to go through the tests' tests yet, but have some nitpicks inline below.

[Martin Albisetti (beuno)]

312 + if 'push-helper' not in self.manifest['hooks'][app]:
313 + # self._add_result(t, n, s)
314 + continue

Why the commenting out?
(the same a few lines lower)

LGTM otherwise.

review: Approve

[lp:~jdstrand/click-reviewers-tools/lp1346481]

228. By Jamie Strandboge on 2014-07-24

clickreviews/cr_security.py: remove confusing comment from
check_template_push_helpers()

229. By Jamie Strandboge on 2014-07-24

clickreviews/cr_security.py: remove confusing comment from
 check_policy_groups_push_helpers()

[Jamie Strandboge (jdstrand)]

> 312 + if 'push-helper' not in self.manifest['hooks'][app]:
> 313 + # self._add_result(t, n, s)
> 314 + continue
>
> Why the commenting out?
> (the same a few lines lower)
>
Ah, these should have been omitted from the commit. Fixed. (There is no reason to run this test if the app doesn't specify a push-helper hook).

>
> LGTM otherwise.

Thanks!

[John Lenton (chipaca)]

review: Approve

[Jamie Strandboge (jdstrand)]

Thanks for the reviews! :)

=== added file 'bin/click-check-push-helper'
--- bin/click-check-push-helper	1970-01-01 00:00:00 +0000
+++ bin/click-check-push-helper	2014-07-24 21:41:48 +0000
@@ -0,0 +1,31 @@
+#!/usr/bin/python3
+'''click-check-push-helper: perform click push-helper checks'''
+#
+# Copyright (C) 2014 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 3 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from __future__ import print_function
+import sys
+
+import clickreviews.cr_common as cr_common
+import clickreviews.cr_push_helper as cr_push_helper
+
+if __name__ == "__main__":
+    if len(sys.argv) < 2:
+        cr_common.error("Must give path to click package")
+
+    review = cr_push_helper.ClickReviewPushHelper(sys.argv[1])
+    review.do_checks()
+    rc = review.do_report()
+    sys.exit(rc)
 
=== modified file 'bin/click-run-checks'
--- bin/click-run-checks	2014-07-16 17:56:05 +0000
+++ bin/click-run-checks	2014-07-24 21:41:48 +0000
@@ -56,6 +56,10 @@
 prefer_local click-check-online-accounts "$c"
 
 echo ""
+echo "= click-check-push-helper ="
+prefer_local click-check-push-helper "$c"
+
+echo ""
 echo "= click-check-scope ="
 prefer_local click-check-scope "$c"
 
 
=== modified file 'bin/click-show-files'
--- bin/click-show-files	2014-07-18 14:18:06 +0000
+++ bin/click-show-files	2014-07-24 21:41:48 +0000
@@ -27,6 +27,7 @@
 from clickreviews import cr_scope
 from clickreviews import cr_content_hub
 from clickreviews import cr_online_accounts
+from clickreviews import cr_push_helper
 
 # This script just dumps important files to stdout
 
@@ -86,6 +87,18 @@
             print("")
     cr_common.cleanup_unpack()
 
+    review_push_helper = cr_push_helper.ClickReviewPushHelper(sys.argv[1])
+    for app in sorted(review_push_helper.push_helper_files):
+        f = review_push_helper.push_helper_files[app]
+        fh = cr_common.open_file_read(os.path.join(
+            review_push_helper.unpack_dir, f))
+        print("== push_helper: %s ==" % os.path.basename(f))
+        for line in fh.readlines():
+            print(line, end="")
+        fh.close()
+        print("")
+    cr_common.cleanup_unpack()
+
     review_scope = cr_scope.ClickReviewScope(sys.argv[1])
     for app in sorted(review_scope.scopes):
         f = review_scope.scopes[app]["ini_file"]
 
=== modified file 'clickreviews/cr_common.py'
--- clickreviews/cr_common.py	2014-06-24 13:51:19 +0000
+++ clickreviews/cr_common.py	2014-07-24 21:41:48 +0000
@@ -62,7 +62,8 @@
         if not self.click_package.endswith(".click"):
             if self.click_package.endswith(".deb"):
                 error("filename does not end with '.click', but '.deb' "
-    "instead. See http://askubuntu.com/a/485544/94326 for how click packages are different.")
+                      "instead. See http://askubuntu.com/a/485544/94326 for "
+                      "how click packages are different.")
             error("filename does not end with '.click'")
 
         self.review_type = review_type
 
=== modified file 'clickreviews/cr_desktop.py'
--- clickreviews/cr_desktop.py	2014-05-30 08:33:09 +0000
+++ clickreviews/cr_desktop.py	2014-07-24 21:41:48 +0000
@@ -37,7 +37,11 @@
         for app in self.manifest['hooks']:
             if 'desktop' not in self.manifest['hooks'][app]:
                 if 'scope' in self.manifest['hooks'][app]:
-                    msg("Skipped missing desktop hook for scope '%s'" % app)
+                    # msg("Skipped missing desktop hook for scope '%s'" % app)
+                    continue
+                if 'push-helper' in self.manifest['hooks'][app]:
+                    # msg("Skipped missing desktop hook for push-helper '%s'" %
+                    #     app)
                     continue
                 else:
                     error("could not find desktop hook for '%s'" % app)
 
=== modified file 'clickreviews/cr_lint.py'
--- clickreviews/cr_lint.py	2014-07-16 19:07:21 +0000
+++ clickreviews/cr_lint.py	2014-07-24 21:41:48 +0000
@@ -69,6 +69,7 @@
                             'content-hub',
                             'desktop',
                             'pay-ui',
+                            'push-helper',
                             'scope',
                             'urls']
 
@@ -281,11 +282,20 @@
         # Some checks are already handled in
         # cr_common.py:_verify_manifest_structure()
 
-        # We don't support multiple apps in 13.10
-        if len(self.manifest['hooks'].keys()) != 1:
-            self._add_result('error', 'hooks',
-                             "more than one app key specified in hooks")
-            return
+        # While we support multiple apps in the hooks db, we don't support
+        # multiple apps specifying desktop hooks. Eg, it is ok to specify a
+        # scope, an app and a push-helper, but it isn't ok to specify two apps
+        t = 'info'
+        n = 'hooks_multiple_apps'
+        s = 'OK'
+        count = 0
+        for app in self.manifest['hooks']:
+            if "desktop" in self.manifest['hooks'][app]:
+                count += 1
+        if count > 1:
+            t = 'error'
+            s = 'more than one desktop app specified in hooks'
+        self._add_result(t, n, s)
 
         # Verify keys are well-formatted
         for app in self.manifest['hooks']:
 
=== added file 'clickreviews/cr_push_helper.py'
--- clickreviews/cr_push_helper.py	1970-01-01 00:00:00 +0000
+++ clickreviews/cr_push_helper.py	2014-07-24 21:41:48 +0000
@@ -0,0 +1,132 @@
+'''cr_push_helper.py: click push-helper checks'''
+#
+# Copyright (C) 2014 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 3 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from __future__ import print_function
+
+from clickreviews.cr_common import ClickReview, error, open_file_read, msg
+import json
+import os
+
+
+class ClickReviewPushHelper(ClickReview):
+    '''This class represents click lint reviews'''
+    def __init__(self, fn):
+        ClickReview.__init__(self, fn, "push_helper")
+
+        self.required_keys = ['exec']
+        self.optional_keys = ['app_id']
+
+        self.push_helper_files = dict()  # click-show-files and tests
+        self.push_helper = dict()
+        for app in self.manifest['hooks']:
+            if 'push-helper' not in self.manifest['hooks'][app]:
+                # msg("Skipped missing push-helper hook for '%s'" % app)
+                continue
+            if not isinstance(self.manifest['hooks'][app]['push-helper'], str):
+                error("manifest malformed: hooks/%s/urls is not str" % app)
+            (full_fn, jd) = self._extract_push_helper(app)
+            self.push_helper_files[app] = full_fn
+            self.push_helper[app] = jd
+
+    def _extract_push_helper(self, app):
+        '''Get push-helper hook content'''
+        c = self.manifest['hooks'][app]['push-helper']
+        fn = os.path.join(self.unpack_dir, c)
+
+        bn = os.path.basename(fn)
+        if not os.path.exists(fn):
+            error("Could not find '%s'" % bn)
+
+        fh = open_file_read(fn)
+        contents = ""
+        for line in fh.readlines():
+                contents += line
+        fh.close()
+
+        try:
+            jd = json.loads(contents)
+        except Exception as e:
+            error("push-helper json unparseable: %s (%s):\n%s" % (bn,
+                  str(e), contents))
+
+        if not isinstance(jd, dict):
+            error("push-helper json is malformed: %s:\n%s" % (bn, contents))
+
+        return (fn, jd)
+
+    def check_valid(self):
+        '''Check validity of push-helper entries'''
+        for app in sorted(self.push_helper):
+            for k in self.push_helper[app].keys():
+                t = "info"
+                n = "valid_%s_%s" % (app, k)
+                s = "OK"
+
+                if not isinstance(self.push_helper[app][k], str):
+                    t = "error"
+                    s = "'%s' is not a string" % k
+                elif self.push_helper[app][k] == "":
+                    t = "error"
+                    s = "'%s' is empty" % k
+                self._add_result(t, n, s)
+
+            for k in self.required_keys:
+                t = "info"
+                n = "valid_%s_required_%s" % (app, k)
+                s = "OK"
+                if k not in self.push_helper[app]:
+                    t = "error"
+                    s = "'%s' is missing" % k
+                self._add_result(t, n, s)
+
+    def check_unknown_keys(self):
+        '''Check unknown'''
+        for app in sorted(self.push_helper):
+            unknown = []
+            t = "info"
+            n = "unknown_%s" % app
+            s = "OK"
+            for key in self.push_helper[app].keys():
+                if key not in self.required_keys and \
+                   key not in self.optional_keys:
+                    unknown.append(key)
+            if len(unknown) == 1:
+                t = "warn"
+                s = "Unknown field '%s'" % unknown[0]
+            elif len(unknown) > 1:
+                t = "warn"
+                s = "Unknown fields '%s'" % ", ".join(unknown)
+            self._add_result(t, n, s)
+
+    def check_hooks(self):
+        '''Verify combinations of click hooks with the push-helper hook'''
+        for app in sorted(self.manifest['hooks']):
+            if app not in self.push_helper:
+                continue
+
+            t = "info"
+            n = "other_hooks_%s" % app
+            s = "OK"
+
+            bad = []
+            for hook in self.manifest['hooks'][app]:
+                # Only the apparmor hook can be used with the push-helper
+                if hook not in ['push-helper', 'apparmor']:
+                    bad.append(hook)
+            if len(bad) > 0:
+                t = 'error'
+                s = "Found unusual hooks with push-helper: %s" % ", ".join(bad)
+            self._add_result(t, n, s)
 
=== modified file 'clickreviews/cr_security.py'
--- clickreviews/cr_security.py	2014-07-18 19:47:48 +0000
+++ clickreviews/cr_security.py	2014-07-24 21:41:48 +0000
@@ -66,6 +66,8 @@
                                              'video',
                                              'webview']
 
+        self.allowed_push_helper_policy_groups = ['push-notification-client']
+
         self.redflag_templates = ['unconfined']
         self.extraneous_templates = ['ubuntu-sdk',
                                      'default']
@@ -357,6 +359,44 @@
 
             self._add_result(t, n, s)
 
+    def check_template_push_helpers(self):
+        '''Check template for push-helpers'''
+        for app in sorted(self.manifest['hooks']):
+            (f, m) = self._get_security_manifest(app)
+            t = 'info'
+            n = 'template_push_helper(%s)' % f
+            s = "OK"
+            if 'push-helper' not in self.manifest['hooks'][app]:
+                continue
+            if 'template' in m and m['template'] != "ubuntu-sdk":
+                t = 'error'
+                s = "template is not 'ubuntu-sdk'"
+            self._add_result(t, n, s)
+
+    def check_policy_groups_push_helpers(self):
+        '''Check policy_groups for push-helpers'''
+        for app in sorted(self.manifest['hooks']):
+            (f, m) = self._get_security_manifest(app)
+            t = 'info'
+            n = 'policy_groups_push_helper(%s)' % f
+            s = "OK"
+            if 'push-helper' not in self.manifest['hooks'][app]:
+                continue
+            if 'policy_groups' not in m or \
+               'push-notification-client' not in m['policy_groups']:
+                self._add_result('error', n,
+                                 "required group 'push-notification-client' "
+                                 "not found")
+                continue
+            bad = []
+            for p in m['policy_groups']:
+                if p not in self.allowed_push_helper_policy_groups:
+                    bad.append(p)
+            if len(bad) > 0:
+                t = 'error'
+                s = "found unusual policy groups: %s" % ", ".join(bad)
+            self._add_result(t, n, s)
+
     def check_policy_groups_scopes(self):
         '''Check policy_groups for scopes'''
         for app in sorted(self.manifest['hooks']):
@@ -397,7 +437,7 @@
             (f, m) = self._get_security_manifest(app)
 
             t = 'info'
-            n = 'policy_groups_exists (%s)' % f
+            n = 'policy_groups_exists_%s (%s)' % (app, f)
             if 'policy_groups' not in m:
                 # If template not specified, we just use the default
                 self._add_result('warn', n, 'no policy groups specified')
@@ -423,7 +463,7 @@
 
             # Check for duplicates
             t = 'info'
-            n = 'policy_groups_duplicates (%s)' % f
+            n = 'policy_groups_duplicates_%s (%s)' % (app, f)
             s = 'OK'
             tmp = []
             for p in m['policy_groups']:
@@ -438,7 +478,7 @@
             # If we got here, we can see if valid policy groups were specified
             for i in m['policy_groups']:
                 t = 'info'
-                n = 'policy_groups_valid (%s)' % i
+                n = 'policy_groups_valid_%s (%s)' % (app, i)
                 s = 'OK'
 
                 # SDK will leave and empty policy group, report but don't
@@ -461,7 +501,7 @@
 
                 if found:
                     t = 'info'
-                    n = 'policy_groups_safe (%s)' % i
+                    n = 'policy_groups_safe_%s (%s)' % (app, i)
                     s = 'OK'
 
                     aa_type = self._get_policy_group_type(vendor, version, i)
 
=== modified file 'clickreviews/cr_tests.py'
--- clickreviews/cr_tests.py	2014-07-16 18:13:22 +0000
+++ clickreviews/cr_tests.py	2014-07-24 21:41:48 +0000
@@ -39,6 +39,7 @@
 TEST_ACCOUNTS_PROVIDER = dict()
 TEST_ACCOUNTS_QML_PLUGIN = dict()
 TEST_ACCOUNTS_SERVICE = dict()
+TEST_PUSH_HELPER = dict()
 
 
 #
@@ -148,6 +149,11 @@
     return (f, val)
 
 
+def _extract_push_helper(self, app):
+    '''Pretend we read the push-helper file'''
+    return ("%s.push-helper.json" % app, TEST_PUSH_HELPER[app])
+
+
 # http://docs.python.org/3.4/library/unittest.mock-examples.html
 # Mock patching. Don't use decorators but instead patch in setUp() of the
 # child. Set up a list of patches, but don't start them. Create the helper
@@ -228,6 +234,11 @@
     'clickreviews.cr_online_accounts.ClickReviewAccounts._extract_account',
     _extract_account))
 
+# push-helper overrides
+patches.append(patch(
+    'clickreviews.cr_push_helper.ClickReviewPushHelper._extract_push_helper',
+    _extract_push_helper))
+
 
 def mock_patch():
     '''Call in setup of child'''
@@ -295,6 +306,7 @@
         self.test_accounts_provider = dict()
         self.test_accounts_qml_plugin = dict()
         self.test_accounts_service = dict()
+        self.test_push_helper = dict()
         for app in self.test_manifest["hooks"].keys():
             # setup security manifest for each app
             self.set_test_security_manifest(app, 'policy_groups',
@@ -330,6 +342,9 @@
             self.set_test_account(app, "account-qml-plugin", None)
             self.set_test_account(app, "account-service", None)
 
+            # Reset to no content-hub entries in manifest
+            self.set_test_push_helper(app, None, None)
+
         self._update_test_security_manifests()
         self._update_test_desktop_files()
         self._update_test_url_dispatcher()
@@ -339,6 +354,7 @@
         self._update_test_accounts_provider()
         self._update_test_accounts_qml_plugin()
         self._update_test_accounts_service()
+        self._update_test_push_helper()
 
         # webapps manifests (leave empty for now)
         self.test_webapp_manifests = dict()
@@ -444,6 +460,15 @@
                 "%s.service" % app
         self._update_test_manifest()
 
+    def _update_test_push_helper(self):
+        global TEST_PUSH_HELPER
+        TEST_PUSH_HELPER = dict()
+        for app in self.test_push_helper.keys():
+            TEST_PUSH_HELPER[app] = self.test_push_helper[app]
+            self.test_manifest["hooks"][app]["push-helper"] = \
+                "%s.push-helper.json" % app
+        self._update_test_manifest()
+
     def _update_test_name(self):
         self.test_name = "%s_%s_%s.click" % (self.test_control['Package'],
                                              self.test_control['Version'],
@@ -618,6 +643,21 @@
         elif account_type == "account-service":
             self._update_test_accounts_service()
 
+    def set_test_push_helper(self, app, key, value):
+        '''Set push-helper entries. If value is None, remove key, if key is
+           None, remove content-hub from manifest'''
+        if key is None:
+            if app in self.test_push_helper:
+                self.test_push_helper.pop(app)
+        elif value is None:
+            if key in self.test_push_helper[app]:
+                self.test_push_helper[app].pop(key)
+        else:
+            if app not in self.test_push_helper:
+                self.test_push_helper[app] = dict()
+            self.test_push_helper[app][key] = value
+        self._update_test_push_helper()
+
     def setUp(self):
         '''Make sure our patches are applied everywhere'''
         global patches
@@ -647,7 +687,9 @@
         global TEST_ACCOUNTS_QML_PLUGIN
         TEST_ACCOUNTS_QML_PLUGIN = dict()
         global TEST_ACCOUNTS_SERVICE
-        TEST_ACCOUNTS_APPLICTION = dict()
+        TEST_ACCOUNTS_APPLICATION = dict()
+        global TEST_PUSH_HELPER
+        TEST_PUSH_HELPER = dict()
 
         self._reset_test_data()
         cr_common.recursive_rm(self.desktop_tmpdir)
 
=== modified file 'clickreviews/tests/test_cr_lint.py'
--- clickreviews/tests/test_cr_lint.py	2014-07-16 14:48:39 +0000
+++ clickreviews/tests/test_cr_lint.py	2014-07-24 21:41:48 +0000
@@ -644,15 +644,37 @@
         expected_counts = {'info': None, 'warn': 0, 'error': 0}
         self.check_results(r, expected_counts)
 
+    def test_check_hooks_multiple_desktop_apps(self):
+        '''Test check_hooks() - multiple desktop apps'''
+        self.set_test_manifest("framework", "ubuntu-sdk-13.10")
+        c = ClickReviewLint(self.test_name)
+        tmp = c.manifest['hooks'][self.default_appname]
+        c.manifest['hooks']["another-app"] = tmp
+        c.check_hooks()
+        r = c.click_report
+        expected_counts = {'info': None, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
     def test_check_hooks_multiple_apps(self):
-        '''Test check_hooks() - multiple apps'''
+        '''Test check_hooks() - multiple non-desktop apps'''
         self.set_test_manifest("framework", "ubuntu-sdk-13.10")
         c = ClickReviewLint(self.test_name)
-        tmp = c.manifest['hooks'][self.default_appname]
-        c.manifest['hooks']["another-app"] = tmp
+        tmp = dict()
+        for k in c.manifest['hooks'][self.default_appname].keys():
+            tmp[k] = c.manifest['hooks'][self.default_appname][k]
+        tmp.pop('desktop')
+        tmp['scope'] = "some-scope-exec"
+        c.manifest['hooks']["some-scope"] = tmp
+        tmp = dict()
+        for k in c.manifest['hooks'][self.default_appname].keys():
+            tmp[k] = c.manifest['hooks'][self.default_appname][k]
+        tmp.pop('desktop')
+        tmp['push-helper'] = "push.json"
+        c.manifest['hooks']["some-push-helper"] = tmp
+
         c.check_hooks()
         r = c.click_report
-        expected_counts = {'info': None, 'warn': 0, 'error': 1}
+        expected_counts = {'info': 7, 'warn': 0, 'error': 0}
         self.check_results(r, expected_counts)
 
     def test_check_hooks_bad_appname(self):
 
=== added file 'clickreviews/tests/test_cr_push_helper.py'
--- clickreviews/tests/test_cr_push_helper.py	1970-01-01 00:00:00 +0000
+++ clickreviews/tests/test_cr_push_helper.py	2014-07-24 21:41:48 +0000
@@ -0,0 +1,138 @@
+'''test_cr_push_helper.py: tests for the cr_push_helper module'''
+#
+# Copyright (C) 2013 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 3 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from clickreviews.cr_push_helper import ClickReviewPushHelper
+import clickreviews.cr_tests as cr_tests
+
+
+class TestClickReviewPushHelper(cr_tests.TestClickReview):
+    """Tests for the lint review tool."""
+    def setUp(self):
+        # Monkey patch various file access classes. stop() is handled with
+        # addCleanup in super()
+        cr_tests.mock_patch()
+        super()
+
+    def test_check_unknown_keys_none(self):
+        '''Test check_unknown() - no unknown'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        c = ClickReviewPushHelper(self.test_name)
+        c.check_unknown_keys()
+        r = c.click_report
+        expected_counts = {'info': 1, 'warn': 0, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_unknown_keys1(self):
+        '''Test check_unknown() - one unknown'''
+        self.set_test_push_helper(self.default_appname, "nonexistent", "foo")
+        c = ClickReviewPushHelper(self.test_name)
+        c.check_unknown_keys()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 1, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_unknown_keys2(self):
+        '''Test check_unknown() - good with one unknown'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        self.set_test_push_helper(self.default_appname, "nonexistent", "foo")
+        c = ClickReviewPushHelper(self.test_name)
+        c.check_unknown_keys()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 1, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_valid_exec(self):
+        '''Test check_valid() - exec'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        c = ClickReviewPushHelper(self.test_name)
+        c.check_valid()
+        r = c.click_report
+        expected_counts = {'info': 2, 'warn': 0, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_valid_missing_exec(self):
+        '''Test check_valid() - missing exec'''
+        self.set_test_push_helper(self.default_appname, "app_id", "foo_foo")
+        c = ClickReviewPushHelper(self.test_name)
+        c.check_valid()
+        r = c.click_report
+        expected_counts = {'info': None, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_valid_app_id(self):
+        '''Test check_valid() - app_id'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        self.set_test_push_helper(self.default_appname, "app_id", "foo_foo")
+        c = ClickReviewPushHelper(self.test_name)
+        c.check_valid()
+        r = c.click_report
+        expected_counts = {'info': 3, 'warn': 0, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_valid_bad_value(self):
+        '''Test check_valid() - bad value'''
+        self.set_test_push_helper(self.default_appname, "exec", [])
+        c = ClickReviewPushHelper(self.test_name)
+        c.check_valid()
+        r = c.click_report
+        expected_counts = {'info': None, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_valid_empty_value(self):
+        '''Test check_valid() - empty value'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        self.set_test_push_helper(self.default_appname, "app_id", "")
+        c = ClickReviewPushHelper(self.test_name)
+        c.check_valid()
+        r = c.click_report
+        expected_counts = {'info': None, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_valid_empty_value2(self):
+        '''Test check_valid() - empty value'''
+        self.set_test_push_helper(self.default_appname, "exec", "")
+        self.set_test_push_helper(self.default_appname, "app_id", "foo_foo")
+        c = ClickReviewPushHelper(self.test_name)
+        c.check_valid()
+        r = c.click_report
+        expected_counts = {'info': None, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_hooks(self):
+        '''Test check_hooks()'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        c = ClickReviewPushHelper(self.test_name)
+
+        # remove hooks that are added by the framework
+        c.manifest['hooks'][self.default_appname].pop('desktop')
+        c.manifest['hooks'][self.default_appname].pop('urls')
+
+        c.check_hooks()
+        r = c.click_report
+        expected_counts = {'info': 1, 'warn': 0, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_hooks_bad(self):
+        '''Test check_hooks() - bad'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        c = ClickReviewPushHelper(self.test_name)
+
+        # The desktop and urls hooks are specified by default in the framework,
+        # so just running this without other setup should generate an error
+        c.check_hooks()
+        r = c.click_report
+        expected_counts = {'info': None, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
 
=== modified file 'clickreviews/tests/test_cr_security.py'
--- clickreviews/tests/test_cr_security.py	2014-07-14 16:40:36 +0000
+++ clickreviews/tests/test_cr_security.py	2014-07-24 21:41:48 +0000
@@ -600,3 +600,87 @@
         report = c.click_report
         expected_counts = {'info': None, 'warn': 0, 'error': 1}
         self.check_results(report, expected_counts)
+
+    def test_check_policy_groups_pushhelper_no_hook(self):
+        '''Test check_policy_groups_pushhelper() - no hook'''
+        c = ClickReviewSecurity(self.test_name)
+        c.check_policy_groups_push_helpers()
+        report = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 0}
+        self.check_results(report, expected_counts)
+
+    def test_check_policy_groups_pushhelper(self):
+        '''Test check_policy_groups_pushhelper()'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        self.set_test_security_manifest(self.default_appname,
+                                        "policy_groups",
+                                        ["push-notification-client"])
+        c = ClickReviewSecurity(self.test_name)
+        c.check_policy_groups_push_helpers()
+        report = c.click_report
+        expected_counts = {'info': 1, 'warn': 0, 'error': 0}
+        self.check_results(report, expected_counts)
+
+    def test_check_policy_groups_pushhelper_missing(self):
+        '''Test check_policy_groups_pushhelper - missing'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        self.set_test_security_manifest(self.default_appname,
+                                        "policy_groups",
+                                        None)
+        c = ClickReviewSecurity(self.test_name)
+        c.check_policy_groups_push_helpers()
+        report = c.click_report
+        expected_counts = {'info': None, 'warn': 0, 'error': 1}
+        self.check_results(report, expected_counts)
+
+    def test_check_policy_groups_pushhelper_bad(self):
+        '''Test check_policy_groups_pushhelper - bad'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        self.set_test_security_manifest(self.default_appname,
+                                        "policy_groups",
+                                        ["video_files",
+                                         "networking",
+                                         "push-notification-client"])
+        c = ClickReviewSecurity(self.test_name)
+        c.check_policy_groups_push_helpers()
+        report = c.click_report
+        expected_counts = {'info': None, 'warn': 0, 'error': 1}
+        self.check_results(report, expected_counts)
+
+    def test_check_template_pushhelper(self):
+        '''Test check_template_pushhelper'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        self.set_test_security_manifest(self.default_appname,
+                                        "template", "ubuntu-sdk")
+        self.set_test_security_manifest(self.default_appname,
+                                        "policy_groups",
+                                        ["push-notification-client"])
+        c = ClickReviewSecurity(self.test_name)
+        c.check_template_push_helpers()
+        report = c.click_report
+        expected_counts = {'info': 1, 'warn': 0, 'error': 0}
+        self.check_results(report, expected_counts)
+
+    def test_check_template_pushhelper_no_hook(self):
+        '''Test check_template_pushhelper'''
+        self.set_test_security_manifest(self.default_appname,
+                                        "template", "ubuntu-sdk")
+        c = ClickReviewSecurity(self.test_name)
+        c.check_template_push_helpers()
+        report = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 0}
+        self.check_results(report, expected_counts)
+
+    def test_check_template_pushhelper_wrong_template(self):
+        '''Test check_template_pushhelper - wrong template()'''
+        self.set_test_push_helper(self.default_appname, "exec", "foo")
+        self.set_test_security_manifest(self.default_appname,
+                                        "template", "ubuntu-webapp")
+        self.set_test_security_manifest(self.default_appname,
+                                        "policy_groups",
+                                        ["push-notification-client"])
+        c = ClickReviewSecurity(self.test_name)
+        c.check_template_push_helpers()
+        report = c.click_report
+        expected_counts = {'info': None, 'warn': 0, 'error': 1}
+        self.check_results(report, expected_counts)
 
=== modified file 'debian/changelog'
--- debian/changelog	2014-07-23 07:53:35 +0000
+++ debian/changelog	2014-07-24 21:41:48 +0000
@@ -26,11 +26,12 @@
     group in 1.2 (LP: #1340869)
   * refactor the way we handle apparmor policy into a central static list
     which should be easy to update.
+  * implement push-helper tests
 
   [ Daniel Holbach ]
   * refer to documentation about click in case we encounter .deb packages.
 
- -- Jamie Strandboge <[e-mail]>  Fri, 18 Jul 2014 14:51:46 -0500
+ -- Jamie Strandboge <[e-mail]>  Thu, 24 Jul 2014 14:04:23 -0500
 
 click-reviewers-tools (0.7.1) utopic; urgency=medium