Merge lp:~chris.gagnon/autopilot/lp1183618 into lp:autopilot

[Chris Gagnon]

don't require -r if -rd is used fixes lp:1183618

don't require -r if -rd is used

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:220
http://jenkins.qa.ubuntu.com/job/autopilot-ci/102/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-saucy-amd64-ci/30/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-saucy-armhf-ci/30/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/autopilot-ci/102/rebuild

review: Needs Fixing (continuous-integration)

[lp:~chris.gagnon/autopilot/lp1183618]

221. By Chris Gagnon on 2013-05-24

remove unneeded record_directory action

[Thomi Richards (thomir-deactivatedaccount)]

A few things.

1)

PEP8 says to have 2 blank lines between module-level functions (well, actually between all blocks at module-level), so the removal of the blank line at diff line 8 should be un-done.

2)

This doesn't make sense:

17 help="Directory to put recorded tests \
18 - (only if -r) specified.")
19 + specified.")

3) You have merge conflict markers in the source.

4) Finally, it seems that you're moifying an existing test, rather than creating a new one for this new feature. I think we need a test that specifies '-rd /some/path' ONLY and then verifies that the video files exist.

Cheers,

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:221
http://jenkins.qa.ubuntu.com/job/autopilot-ci/103/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-saucy-amd64-ci/31/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-saucy-armhf-ci/31/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/autopilot-ci/103/rebuild

review: Needs Fixing (continuous-integration)

[lp:~chris.gagnon/autopilot/lp1183618]

222. By Chris Gagnon on 2013-05-24

undo deleted line

223. By Chris Gagnon on 2013-05-24

fixed merge conflict

[Chris Gagnon (chris.gagnon)]

1. is fixed, I use flake8 with vim for pep8 checking, but there are so many errors the new one was hard to see.
2. is fixed
3. is fixed
4. the test without -r was using -rd, I had to modify the test so it would pass. because -r is not required when using -rd. there is already a test for -rd path and -r using the default of /tmp/autopilot

thanks

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:223
http://jenkins.qa.ubuntu.com/job/autopilot-ci/104/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-saucy-amd64-ci/32
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-saucy-armhf-ci/32

Click here to trigger a rebuild:
http://s-jenkins:8080/job/autopilot-ci/104/rebuild

review: Approve (continuous-integration)

[Thomi Richards (thomir-deactivatedaccount)]

review: Approve

=== modified file 'autopilot/__init__.py'
--- autopilot/__init__.py	2013-05-22 20:43:20 +0000
+++ autopilot/__init__.py	2013-05-24 05:04:28 +0000
@@ -59,9 +59,7 @@
                             'recordmydesktop' app to be installed.\
                             Videos are stored in /tmp/autopilot.")
     parser_run.add_argument("-rd", "--record-directory", required=False,
-                            default="/tmp/autopilot", type=str,
-                            help="Directory to put recorded tests \
-                            (only if -r) specified.")
+                            help="Directory to put recorded tests")
     parser_run.add_argument("-ro", "--random-order", action='store_true',
                             required=False, default=False,
                             help="Run the tests in random order")

=== modified file 'autopilot/tests/functional/test_autopilot_functional.py'
--- autopilot/tests/functional/test_autopilot_functional.py	2013-05-24 00:00:06 +0000
+++ autopilot/tests/functional/test_autopilot_functional.py	2013-05-24 05:04:28 +0000
@@ -426,6 +426,12 @@
             ))
 
         should_delete = not os.path.exists('/tmp/autopilot')
+        if should_delete:
+            self.addCleanup(remove_if_exists, "/tmp/autopilot")
+        else:
+            self.addCleanup(remove_if_exists, 
+                            '/tmp/autopilot/tests.test_simple.SimpleTest.test_simple.ogv')
+
         code, output, error = self.run_autopilot(["run", "-r", "tests"])
 
         self.assertThat(code, Equals(1))
@@ -466,22 +472,23 @@
         self.create_test_file("test_simple.py", dedent("""\
 
             from autopilot.testcase import AutopilotTestCase
-
+            from time import sleep
 
             class SimpleTest(AutopilotTestCase):
 
                 def test_simple(self):
+                    sleep(1)
                     self.fail()
             """
             ))
-        video_dir = mktemp()
-        self.addCleanup(remove_if_exists, video_dir)
+        self.addCleanup(remove_if_exists,
+            '/tmp/autopilot/tests.test_simple.SimpleTest.test_simple.ogv')
 
-        code, output, error = self.run_autopilot(["run", "-rd", video_dir, "tests"])
+        code, output, error = self.run_autopilot(["run", "tests"])
 
         self.assertThat(code, Equals(1))
-        self.assertFalse(os.path.exists(video_dir))
-        self.assertFalse(os.path.exists('%s/tests.test_simple.SimpleTest.test_simple.ogv' % (video_dir)))
+        self.assertFalse(os.path.exists(
+            '/tmp/autopilot/tests.test_simple.SimpleTest.test_simple.ogv'))
 
     def test_runs_with_import_errors_fail(self):
         """Import errors inside a test must be considered a test failure."""

=== modified file 'autopilot/tests/unit/test_command_line_args.py'
--- autopilot/tests/unit/test_command_line_args.py	2013-05-22 20:43:20 +0000
+++ autopilot/tests/unit/test_command_line_args.py	2013-05-24 05:04:28 +0000
@@ -220,10 +220,6 @@
         args = self.parse_args("run --record foo")
         self.assertThat(args.record, Equals(True))
 
-    def test_run_command_record_dir_flag_default(self):
-        args = self.parse_args("run foo")
-        self.assertThat(args.record_directory, Equals("/tmp/autopilot"))
-
     def test_run_command_record_dir_flag_short(self):
         args = self.parse_args("run -rd /path/to/dir foo")
         self.assertThat(args.record_directory, Equals("/path/to/dir"))

=== modified file 'bin/autopilot'
--- bin/autopilot	2013-05-22 20:43:20 +0000
+++ bin/autopilot	2013-05-24 05:04:28 +0000
@@ -90,7 +90,12 @@
 
     import autopilot.globals
 
+    if args.record_directory:
+        args.record = True
+
     if args.record:
+        if not args.record_directory:
+            args.record_directory = '/tmp/autopilot'
         if subprocess.call(['which', 'recordmydesktop'], stdout=subprocess.PIPE) != 0:
             print "ERROR: The application 'recordmydesktop' needs to be installed to record failing jobs."
             exit(1)