Merge lp:~canonical-platform-qa/ubuntu-ui-toolkit/fixture_unset_env into lp:ubuntu-ui-toolkit/staging

[Leo Arias]

In the initctl test fixture, add the option to unset an environment variable.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1508
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1758/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2749/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/486/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/488/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/485/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2747/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1758/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1509
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1759/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2757
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/487
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/489
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/489/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/486
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/2374
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2755
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2755/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20430

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1759/rebuild

review: Needs Fixing (continuous-integration)

[Leo Arias (elopio)]

I've pushed an updated version. Thanks!

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1510
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1761/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2762
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/489
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/491
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/491/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/488
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/2381
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2760
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2760/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20438

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1761/rebuild

review: Needs Fixing (continuous-integration)

[Vincent Ladeuil (vila)]

Scenarios keys, while descriptive, should stay short, see inline comment.

On the overall, and I realize this MP doesn't introduce the issue, the scenarios look far too complex.

https://bazaar.launchpad.net/~canonical-platform-qa/uci-tests/trunk/view/head:/ucitests/tests/test_fixtures.py TestEnv attempts to test something similar with far less code and no scenarios.

May be I'm missing something about what is tested in this MP but the difference between the two is worth discussing.

review: Needs Fixing

[Leo Arias (elopio)]

Yes, I didn't like to write this test. I have an idea that might make it better. Back to work in progress.

[Leo Arias (elopio)]

Back to needs review. Please take another look vila.

[Vincent Ladeuil (vila)]

Far better !

There is still a lot of duplicated code in the tests that should either go to setUp or specific helpers, mostly around defining and running the inner test and check it was successful but the test setup itself could benefit from a helper to highlight the test specifics more clearly (setting or unsetting the env var IIRC).

I tried to find how to run the tests locally before doing that refactoring myself but get lost >-/ Can you help ?

Overall it's already pretty clear so you can land as is but I think that can be made crystal clear without too much effort.

review: Approve

[Leo Arias (elopio)]

Yes, you don't need to do much as this is only python:

bzr branch lp:~canonical-platform-qa/ubuntu-ui-toolkit/fixture_unset_env
cd fixture_unset_env/tests/autopilot
autopilot3 run ubuntuuitoolkit.tests.test_fixture_setup

I want crystal clear, so please show me the refactorings you have in mind if you have some time.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1511
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1766/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2835
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/494
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/496
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/496/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/493
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/2428
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2833
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2833/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20518

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1766/rebuild

review: Needs Fixing (continuous-integration)

[Vincent Ladeuil (vila)]

@Leo: I've pushed the refactoring I was talking about, feel free to revisit.

The resulting diff is not that clear so looking at the branch history may be easier to understand how I got there.

The end result really focus on what the tests do, pushing down (and sharing !) the implementation details into the helpers.

The tests now can ignore that 'testenvvarforfixture' is used internally and focus on what happens to the variable.

This also ensures that all the tests share the details on how they test without the risk of divergence if the tests are modified later.

Funnily enough, this refactoring revealed that one addCleanup() call was useless (see history).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1517
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1769/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2840
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/497
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/499
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/499/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/496
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/2432
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2838
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2838/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20529

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1769/rebuild

review: Needs Fixing (continuous-integration)

[Leo Arias (elopio)]

Wow, 3 lines and 4 lines. Thanks Vila!

[Zoltan Balogh (bzoltan)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'tests/autopilot/ubuntuuitoolkit/fixture_setup.py'
--- tests/autopilot/ubuntuuitoolkit/fixture_setup.py	2015-04-14 21:02:06 +0000
+++ tests/autopilot/ubuntuuitoolkit/fixture_setup.py	2015-05-15 10:13:47 +0000
@@ -127,8 +127,12 @@
         super().setUp()
         for variable, value in self.variables.items():
             self._add_variable_cleanup(variable)
-            self.environment.set_initctl_env_var(
-                variable, value, global_=self.global_)
+            if value is None:
+                self.environment.unset_initctl_env_var(
+                    variable, global_=self.global_)
+            else:
+                self.environment.set_initctl_env_var(
+                    variable, value, global_=self.global_)
 
     def _add_variable_cleanup(self, variable):
         if self.environment.is_initctl_env_var_set(

=== modified file 'tests/autopilot/ubuntuuitoolkit/tests/test_fixture_setup.py'
--- tests/autopilot/ubuntuuitoolkit/tests/test_fixture_setup.py	2015-04-14 21:02:06 +0000
+++ tests/autopilot/ubuntuuitoolkit/tests/test_fixture_setup.py	2015-05-15 10:13:47 +0000
@@ -170,160 +170,99 @@
         self.application.select_single('Label', objectName='testLabel')
 
 
-class InitctlEnvironmentVariableTestCase(testtools.TestCase):
-
-    def test_use_initctl_environment_variable_with_unset_variable(self):
+class InitctlEnvironmentVariableTestCase(testscenarios.TestWithScenarios):
+
+    scenarios = [
+        ('global_variable', {'global_': True}),
+        ('local_variable', {'global_': False})
+    ]
+
+    def set_original_value(self, value):
+        self.addCleanup(
+            environment.unset_initctl_env_var, 'testenvvarforfixture',
+            global_=self.global_)
+        environment.set_initctl_env_var('testenvvarforfixture',
+                                        value, global_=self.global_)
+
+    def create_fixture(self, value):
+        self.initctl_env_var = fixture_setup.InitctlEnvironmentVariable(
+            testenvvarforfixture=value, global_=self.global_)
+
+    def assertValueIs(self, expected_value):
+        self.assertEqual(
+            expected_value,
+            environment.get_initctl_env_var(
+                'testenvvarforfixture', global_=self.global_))
+
+    def assertVariableIsNotSet(self):
+        self.assertFalse(
+            environment.is_initctl_env_var_set(
+                'testenvvarforfixture', global_=self.global_))
+
+    def assertTestIsSuccessful(self, expected_value, test_name):
+        result = testtools.TestResult()
+
+        class TestWithInitctlEnvVar(testtools.TestCase):
+            def setUp(inner):
+                super().setUp()
+                inner.useFixture(self.initctl_env_var)
+
+            def test_value_set(inner):
+                self.assertValueIs(expected_value)
+
+            def test_variable_not_set(inner):
+                self.assertVariableIsNotSet()
+
+        TestWithInitctlEnvVar(test_name).run(result)
+        self.assertTrue(
+            result.wasSuccessful(), 'Failed to set the environment variable.')
+
+    def test_use_initctl_environment_variable_to_set_unexisting_variable(self):
         """Test the initctl env var fixture when the var is unset.
 
         During the test, the new value must be in place.
         After the test, the variable must be unset again.
 
         """
-        initctl_env_var = fixture_setup.InitctlEnvironmentVariable(
-            testenvvarforfixture='test value')
-
-        result = testtools.TestResult()
-
-        def inner_test():
-            class TestWithInitctlEnvVar(testtools.TestCase):
-                def test_it(self):
-                    self.useFixture(initctl_env_var)
-                    self.assertEqual(
-                        'test value',
-                        environment.get_initctl_env_var(
-                            'testenvvarforfixture'))
-            return TestWithInitctlEnvVar('test_it')
-
-        inner_test().run(result)
-
-        self.assertTrue(
-            result.wasSuccessful(), 'Failed to set the environment variable.')
-        self.assertFalse(
-            environment.is_initctl_env_var_set('testenvvarforfixture'))
-
-    def test_use_initctl_environment_variable_with_set_variable(self):
+        self.create_fixture('test value')
+        self.assertTestIsSuccessful('test value', 'test_value_set')
+        self.assertVariableIsNotSet()
+
+    def test_use_initctl_environment_variable_to_set_existing_variable(self):
         """Test the initctl env var fixture when the var is unset.
 
         During the test, the new value must be in place.
         After the test, the old value must be set again.
 
         """
-        self.addCleanup(
-            environment.unset_initctl_env_var, 'testenvvarforfixture')
-        environment.set_initctl_env_var(
-            'testenvvarforfixture', 'original test value')
-
-        initctl_env_var = fixture_setup.InitctlEnvironmentVariable(
-            testenvvarforfixture='new test value')
-
-        result = testtools.TestResult()
-
-        def inner_test():
-            class TestWithInitctlEnvVar(testtools.TestCase):
-                def test_it(self):
-                    self.useFixture(initctl_env_var)
-                    self.assertEqual(
-                        'new test value',
-                        environment.get_initctl_env_var(
-                            'testenvvarforfixture'))
-            return TestWithInitctlEnvVar('test_it')
-
-        inner_test().run(result)
-
-        self.assertTrue(
-            result.wasSuccessful(), 'Failed to set the environment variable.')
-        self.assertEqual(
-            'original test value',
-            environment.get_initctl_env_var('testenvvarforfixture'))
-
-
-class InitctlGlobalEnvironmentVariableTestCase(
-        testscenarios.TestWithScenarios):
-
-    scenarios = [
-        ('global unset variable', {
-            'is_variable_set': False,
-            'variable_value': 'dummy',
-            'global_value': 'value',
-            'expected_calls': [
-                mock.call.is_initctl_env_var_set(
-                    'testenvvarforfixture', global_='value'),
-                mock.call.set_initctl_env_var(
-                    'testenvvarforfixture', 'new test value', global_='value'),
-                mock.call.unset_initctl_env_var(
-                    'testenvvarforfixture', global_='value')
-            ]
-        }),
-        ('global set variable', {
-            'is_variable_set': True,
-            'variable_value': 'original_value',
-            'global_value': 'value',
-            'expected_calls': [
-                mock.call.is_initctl_env_var_set(
-                    'testenvvarforfixture', global_='value'),
-                mock.call.get_initctl_env_var(
-                    'testenvvarforfixture', global_='value'),
-                mock.call.set_initctl_env_var(
-                    'testenvvarforfixture', 'new test value', global_='value'),
-                mock.call.set_initctl_env_var(
-                    'testenvvarforfixture', 'original_value', global_='value')
-            ]
-        }),
-        ('default unset variable', {
-            'is_variable_set': False,
-            'variable_value': 'dummy',
-            'global_value': 'default',
-            'expected_calls': [
-                mock.call.is_initctl_env_var_set(
-                    'testenvvarforfixture', global_=False),
-                mock.call.set_initctl_env_var(
-                    'testenvvarforfixture', 'new test value', global_=False),
-                mock.call.unset_initctl_env_var(
-                    'testenvvarforfixture', global_=False)
-            ]
-        }),
-        ('global set variable', {
-            'is_variable_set': True,
-            'variable_value': 'original_value',
-            'global_value': 'default',
-            'expected_calls': [
-                mock.call.is_initctl_env_var_set(
-                    'testenvvarforfixture', global_=False),
-                mock.call.get_initctl_env_var(
-                    'testenvvarforfixture', global_=False),
-                mock.call.set_initctl_env_var(
-                    'testenvvarforfixture', 'new test value', global_=False),
-                mock.call.set_initctl_env_var(
-                    'testenvvarforfixture', 'original_value', global_=False)
-            ]
-        }),
-    ]
-
-    def test_use_initctl_environment_variable_with_global_unset_variable(self):
-        if self.global_value == 'default':
-            initctl_env_var = fixture_setup.InitctlEnvironmentVariable(
-                testenvvarforfixture='new test value')
-        else:
-            initctl_env_var = fixture_setup.InitctlEnvironmentVariable(
-                testenvvarforfixture='new test value',
-                global_=self.global_value)
-
-        mock_env = mock.Mock()
-        initctl_env_var.environment = mock_env
-        mock_env.is_initctl_env_var_set.return_value = self.is_variable_set
-        mock_env.get_initctl_env_var.return_value = self.variable_value
-
-        def inner_test():
-            class TestWithInitctlEnvVar(testtools.TestCase):
-                def test_it(self):
-                    self.useFixture(initctl_env_var)
-
-            return TestWithInitctlEnvVar('test_it')
-
-        inner_test().run()
-
-        self.assertEquals(
-            self.expected_calls, mock_env.mock_calls)
+        self.set_original_value('original test value')
+        self.create_fixture('new test value')
+        self.assertTestIsSuccessful('new test value', 'test_value_set')
+        self.assertValueIs('original test value')
+
+    def test_use_initctl_environment_variable_to_unset_existing_variable(self):
+        """Test the initctl env var fixture to unset a variable.
+
+        During the test, the variable must be unset.
+        After the test, the old value must be set again.
+
+        """
+        self.set_original_value('original test value')
+        self.create_fixture(None)
+        self.assertTestIsSuccessful(None, 'test_variable_not_set')
+        self.assertValueIs('original test value',)
+
+    def test_use_initctl_environment_variable_to_unset_nonexisting_variable(
+            self):
+        """Test the initctl env var fixture to unset a variable.
+
+        During the test, the variable must be unset.
+        After the test, the variable must remain unset.
+
+        """
+        self.create_fixture(None)
+        self.assertTestIsSuccessful(None, 'test_variable_not_set')
+        self.assertVariableIsNotSet()
 
 
 class FakeHomeTestCase(testtools.TestCase):