Covert all paver command manipulation to happen on lists of args/options, rather than strings

pavelib/paver_tests/test_js_test.py

@@ -137,7 +137,7 @@ class TestPaverJavaScriptTestTasks(PaverTestCase):
                     suite=suite
                 )
             if port:
-                expected_test_tool_command += u" --port {port}".format(port=port)
+                expected_test_tool_command += u" --port={port}".format(port=port)
             expected_messages.append(expected_test_tool_command)
 
         self.assertEquals(self.task_messages, expected_messages)

pavelib/paver_tests/test_paver_bok_choy_cmds.py

@@ -24,25 +24,24 @@ class TestPaverBokChoyCmd(unittest.TestCase):
         and store.
         """
 
-        expected_statement = (
-            "DEFAULT_STORE={default_store} "
-            "SCREENSHOT_DIR='{repo_dir}/test_root/log{shard_str}' "
-            "BOK_CHOY_HAR_DIR='{repo_dir}/test_root/log{shard_str}/hars' "
-            "BOKCHOY_A11Y_CUSTOM_RULES_FILE='{repo_dir}/{a11y_custom_file}' "
-            "SELENIUM_DRIVER_LOG_DIR='{repo_dir}/test_root/log{shard_str}' "
-            "VERIFY_XSS='{verify_xss}' "
-            "nosetests {repo_dir}/common/test/acceptance/{exp_text} "
-            "--with-xunit "
-            "--xunit-file={repo_dir}/reports/bok_choy{shard_str}/xunit.xml "
-            "--verbosity=2 "
-        ).format(
-            default_store=store,
-            repo_dir=REPO_DIR,
-            shard_str='/shard_' + self.shard if self.shard else '',
-            exp_text=name,
-            a11y_custom_file='node_modules/edx-custom-a11y-rules/lib/custom_a11y_rules.js',
-            verify_xss=verify_xss
-        )
+        shard_str = '/shard_' + self.shard if self.shard else ''
+
+        expected_statement = [
+            "DEFAULT_STORE={}".format(store),
+            "SCREENSHOT_DIR='{}/test_root/log{}'".format(REPO_DIR, shard_str),
+            "BOK_CHOY_HAR_DIR='{}/test_root/log{}/hars'".format(REPO_DIR, shard_str),
+            "BOKCHOY_A11Y_CUSTOM_RULES_FILE='{}/{}'".format(
+                REPO_DIR,
+                'node_modules/edx-custom-a11y-rules/lib/custom_a11y_rules.js'
+            ),
+            "SELENIUM_DRIVER_LOG_DIR='{}/test_root/log{}'".format(REPO_DIR, shard_str),
+            "VERIFY_XSS='{}'".format(verify_xss),
+            "nosetests",
+            "{}/common/test/acceptance/{}".format(REPO_DIR, name),
+            "--with-xunit",
+            "--xunit-file={}/reports/bok_choy{}/xunit.xml".format(REPO_DIR, shard_str),
+            "--verbosity=2",
+        ]
         return expected_statement
 
     def setUp(self):
@@ -93,7 +92,7 @@ class TestPaverBokChoyCmd(unittest.TestCase):
 
     def test_serversonly(self):
         suite = BokChoyTestSuite('', serversonly=True)
-        self.assertEqual(suite.cmd, "")
+        self.assertEqual(suite.cmd, None)
 
     def test_verify_xss(self):
         suite = BokChoyTestSuite('', verify_xss=True)
@@ -119,14 +118,16 @@ class TestPaverBokChoyCmd(unittest.TestCase):
         """
         Using 1 process means paver should ask for the traditional xunit plugin for plugin results
         """
-        expected_verbosity_string = (
-            "--with-xunit --xunit-file={repo_dir}/reports/bok_choy{shard_str}/xunit.xml --verbosity=2".format(
+        expected_verbosity_command = [
+            "--with-xunit",
+            "--xunit-file={repo_dir}/reports/bok_choy{shard_str}/xunit.xml".format(
                 repo_dir=REPO_DIR,
                 shard_str='/shard_' + self.shard if self.shard else ''
-            )
-        )
+            ),
+            "--verbosity=2",
+        ]
         suite = BokChoyTestSuite('', num_processes=1)
-        self.assertEqual(BokChoyTestSuite.verbosity_processes_string(suite), expected_verbosity_string)
+        self.assertEqual(suite.verbosity_processes_command, expected_verbosity_command)
 
     def test_verbosity_settings_2_processes(self):
         """
@@ -134,32 +135,36 @@ class TestPaverBokChoyCmd(unittest.TestCase):
         be used.
         """
         process_count = 2
-        expected_verbosity_string = (
-            "--with-xunitmp --xunitmp-file={repo_dir}/reports/bok_choy{shard_str}/xunit.xml"
-            " --processes={procs} --no-color --process-timeout=1200".format(
+        expected_verbosity_command = [
+            "--with-xunitmp",
+            "--xunitmp-file={repo_dir}/reports/bok_choy{shard_str}/xunit.xml".format(
                 repo_dir=REPO_DIR,
                 shard_str='/shard_' + self.shard if self.shard else '',
-                procs=process_count
-            )
-        )
+            ),
+            "--processes={}".format(process_count),
+            "--no-color",
+            "--process-timeout=1200",
+        ]
         suite = BokChoyTestSuite('', num_processes=process_count)
-        self.assertEqual(BokChoyTestSuite.verbosity_processes_string(suite), expected_verbosity_string)
+        self.assertEqual(suite.verbosity_processes_command, expected_verbosity_command)
 
     def test_verbosity_settings_3_processes(self):
         """
         With the above test, validate that num_processes can be set to various values
         """
         process_count = 3
-        expected_verbosity_string = (
-            "--with-xunitmp --xunitmp-file={repo_dir}/reports/bok_choy{shard_str}/xunit.xml"
-            " --processes={procs} --no-color --process-timeout=1200".format(
+        expected_verbosity_command = [
+            "--with-xunitmp",
+            "--xunitmp-file={repo_dir}/reports/bok_choy{shard_str}/xunit.xml".format(
                 repo_dir=REPO_DIR,
                 shard_str='/shard_' + self.shard if self.shard else '',
-                procs=process_count
-            )
-        )
+            ),
+            "--processes={}".format(process_count),
+            "--no-color",
+            "--process-timeout=1200",
+        ]
         suite = BokChoyTestSuite('', num_processes=process_count)
-        self.assertEqual(BokChoyTestSuite.verbosity_processes_string(suite), expected_verbosity_string)
+        self.assertEqual(suite.verbosity_processes_command, expected_verbosity_command)
 
     def test_invalid_verbosity_and_processes(self):
         """
@@ -168,7 +173,8 @@ class TestPaverBokChoyCmd(unittest.TestCase):
         """
         suite = BokChoyTestSuite('', num_processes=2, verbosity=3)
         with self.assertRaises(BuildFailure):
-            BokChoyTestSuite.verbosity_processes_string(suite)
+            # pylint: disable=pointless-statement
+            suite.verbosity_processes_command
 
 
 class TestPaverPa11yCrawlerCmd(unittest.TestCase):
@@ -192,17 +198,16 @@ class TestPaverPa11yCrawlerCmd(unittest.TestCase):
         """
         Returns the expected command to run pa11ycrawler.
         """
-        expected_statement = (
-            'pa11ycrawler run {start_urls} '
-            '--pa11ycrawler-allowed-domains=localhost '
-            '--pa11ycrawler-reports-dir={report_dir} '
-            '--pa11ycrawler-deny-url-matcher=logout '
-            '--pa11y-reporter="1.0-json" '
-            '--depth-limit=6 '
-        ).format(
-            start_urls=' '.join(start_urls),
-            report_dir=report_dir,
-        )
+        expected_statement = [
+            'pa11ycrawler',
+            'run',
+        ] + start_urls + [
+            '--pa11ycrawler-allowed-domains=localhost',
+            '--pa11ycrawler-reports-dir={}'.format(report_dir),
+            '--pa11ycrawler-deny-url-matcher=logout',
+            '--pa11y-reporter="1.0-json"',
+            '--depth-limit=6',
+        ]
         return expected_statement
 
     def test_default(self):

pavelib/utils/test/suites/acceptance_suite.py

@@ -34,22 +34,21 @@ class AcceptanceTest(TestSuite):
     def cmd(self):
 
         report_file = self.report_dir / "{}.xml".format(self.system)
-        report_args = "--with-xunit --xunit-file {}".format(report_file)
-
-        cmd = (
-            "DEFAULT_STORE={default_store} ./manage.py {system} --settings acceptance harvest --traceback "
-            "--debug-mode --verbosity {verbosity} {pdb}{report_args} {extra_args} {passthrough}".format(
-                default_store=self.default_store,
-                system=self.system,
-                verbosity=self.verbosity,
-                pdb="--pdb " if self.pdb else "",
-                report_args=report_args,
-                extra_args=self.extra_args,
-                passthrough=self.passthrough_options
-            )
-        )
-
-        return cmd
+        report_args = ["--with-xunit", "--xunit-file {}".format(report_file)]
+
+        return [
+            "DEFAULT_STORE={}".format(self.default_store),
+            "./manage.py",
+            self.system,
+            "--settings=acceptance",
+            "harvest",
+            "--traceback",
+            "--debug-mode",
+            "--verbosity={}".format(self.verbosity),
+            "--pdb" if self.pdb else "",
+        ] + report_args + [
+            self.extra_args
+        ] + self.passthrough_options
 
     def _update_assets(self):
         """

pavelib/utils/test/suites/bokchoy_suite.py

@@ -118,33 +118,36 @@ class BokChoyTestSuite(TestSuite):
             sh("./manage.py lms --settings bok_choy flush --traceback --noinput")
             bokchoy_utils.clear_mongo()
 
-    def verbosity_processes_string(self):
+    @property
+    def verbosity_processes_command(self):
         """
         Multiprocessing, xunit, color, and verbosity do not work well together. We need to construct
         the proper combination for use with nosetests.
         """
-        substring = []
+        command = []
 
         if self.verbosity != DEFAULT_VERBOSITY and self.num_processes != DEFAULT_NUM_PROCESSES:
             msg = 'Cannot pass in both num_processors and verbosity. Quitting'
             raise BuildFailure(msg)
 
         if self.num_processes != 1:
-            # Construct "multiprocess" nosetest substring
-            substring = [
-                "--with-xunitmp --xunitmp-file={}".format(self.xunit_report),
+            # Construct "multiprocess" nosetest command
+            command = [
+                "--with-xunitmp",
+                "--xunitmp-file={}".format(self.xunit_report),
                 "--processes={}".format(self.num_processes),
-                "--no-color --process-timeout=1200"
+                "--no-color",
+                "--process-timeout=1200",
             ]
 
         else:
-            substring = [
+            command = [
                 "--with-xunit",
                 "--xunit-file={}".format(self.xunit_report),
                 "--verbosity={}".format(self.verbosity),
             ]
 
-        return " ".join(substring)
+        return command
 
     def prepare_bokchoy_run(self):
         """
@@ -224,7 +227,7 @@ class BokChoyTestSuite(TestSuite):
     def cmd(self):
         """
         This method composes the nosetests command to send to the terminal. If nosetests aren't being run,
-         the command returns an empty string.
+         the command returns None.
         """
         # Default to running all tests if no specific test is specified
         if not self.test_spec:
@@ -235,7 +238,7 @@ class BokChoyTestSuite(TestSuite):
         # Skip any additional commands (such as nosetests) if running in
         # servers only mode
         if self.serversonly:
-            return ""
+            return None
 
         # Construct the nosetests command, specifying where to save
         # screenshots and XUnit XML reports
@@ -248,16 +251,15 @@ class BokChoyTestSuite(TestSuite):
             "VERIFY_XSS='{}'".format(self.verify_xss),
             "nosetests",
             test_spec,
-            "{}".format(self.verbosity_processes_string())
-        ]
+        ] + self.verbosity_processes_command
         if self.pdb:
             cmd.append("--pdb")
         if self.save_screenshots:
             cmd.append("--with-save-baseline")
-        cmd.append(self.extra_args)
+        if self.extra_args:
+            cmd.append(self.extra_args)
         cmd.extend(self.passthrough_options)
 
-        cmd = (" ").join(cmd)
         return cmd
 
 
@@ -353,19 +355,14 @@ class Pa11yCrawler(BokChoyTestSuite):
         """
         Runs pa11ycrawler as staff user against the test course.
         """
-        cmd_str = (
-            'pa11ycrawler run {start_urls} '
-            '--pa11ycrawler-allowed-domains={allowed_domains} '
-            '--pa11ycrawler-reports-dir={report_dir} '
-            '--pa11ycrawler-deny-url-matcher={dont_go_here} '
-            '--pa11y-reporter="{reporter}" '
-            '--depth-limit={depth} '
-        ).format(
-            start_urls=' '.join(self.start_urls),
-            allowed_domains='localhost',
-            report_dir=self.pa11y_report_dir,
-            reporter="1.0-json",
-            dont_go_here="logout",
-            depth="6",
-        )
-        return cmd_str
+        cmd = [
+            'pa11ycrawler',
+            'run',
+        ] + self.start_urls + [
+            '--pa11ycrawler-allowed-domains=localhost',
+            '--pa11ycrawler-reports-dir={}'.format(self.pa11y_report_dir),
+            '--pa11ycrawler-deny-url-matcher=logout',
+            '--pa11y-reporter="1.0-json"',
+            '--depth-limit=6',
+        ]
+        return cmd

pavelib/utils/test/suites/js_suite.py

@@ -76,21 +76,22 @@ class JsTestSubSuite(TestSuite):
         """
         Run the tests using karma runner.
         """
-        cmd = (
-            "karma start {test_conf_file} --single-run={single_run} --capture-timeout=60000 "
-            "--junitreportpath={xunit_report}".format(
-                single_run='false' if self.mode == 'dev' else 'true',
-                test_conf_file=self.test_conf_file,
-                xunit_report=self.xunit_report,
-            )
-        )
+        cmd = [
+            "karma",
+            "start",
+            self.test_conf_file,
+            "--single-run={}".format('false' if self.mode == 'dev' else 'true'),
+            "--capture-timeout=60000",
+            "--junitreportpath={}".format(self.xunit_report),
+        ]
 
         if self.port:
-            cmd += " --port {port}".format(port=self.port)
+            cmd.append("--port={}".format(self.port))
 
         if self.run_under_coverage:
-            cmd += " --coverage --coveragereportpath={report_path}".format(
-                report_path=self.coverage_report
-            )
+            cmd.extend([
+                "--coverage",
+                "--coveragereportpath={}".format(self.coverage_report),
+            ])
 
         return cmd

pavelib/utils/test/suites/nose_suite.py

@@ -59,23 +59,21 @@ class NoseTestSuite(TestSuite):
         unaltered otherwise.
         """
         if self.run_under_coverage:
-            cmd0, cmd_rest = cmd.split(" ", 1)
             # We use "python -m coverage" so that the proper python
             # will run the importable coverage rather than the
             # coverage that OS path finds.
 
-            if not cmd0.endswith('.py'):
-                cmd0 = "`which {}`".format(cmd0)
-
-            cmd = (
-                "python -m coverage run {cov_args} --rcfile={rcfile} "
-                "{cmd0} {cmd_rest}".format(
-                    cov_args=self.cov_args,
-                    rcfile=Env.PYTHON_COVERAGERC,
-                    cmd0=cmd0,
-                    cmd_rest=cmd_rest,
-                )
-            )
+            if not cmd[0].endswith('.py'):
+                cmd[0] = "`which {}`".format(cmd[0])
+
+            cmd = [
+                "python",
+                "-m",
+                "coverage",
+                "run",
+                self.cov_args,
+                "--rcfile={}".format(Env.PYTHON_COVERAGERC),
+            ] + cmd
 
         return cmd
 
@@ -85,14 +83,14 @@ class NoseTestSuite(TestSuite):
         Takes the test options and returns the appropriate flags
         for the command.
         """
-        opts = " "
+        opts = []
 
         # Handle "--failed" as a special case: we want to re-run only
         # the tests that failed within our Django apps
         # This sets the --failed flag for the nosetests command, so this
         # functionality is the same as described in the nose documentation
         if self.failed_only:
-            opts += "--failed"
+            opts.append("--failed")
 
         # This makes it so we use nose's fail-fast feature in two cases.
         # Case 1: --fail-fast is passed as an arg in the paver command
@@ -102,13 +100,13 @@ class NoseTestSuite(TestSuite):
         )
 
         if self.fail_fast or env_fail_fast_set:
-            opts += " --stop"
+            opts.append("--stop")
 
         if self.pdb:
             opts += " --pdb"
 
         if self.use_ids:
-            opts += " --with-id"
+            opts.append("--with-id")
 
         return opts
 
@@ -152,7 +150,7 @@ class SystemTestSuite(NoseTestSuite):
             './manage.py', self.root, 'test',
             '--verbosity={}'.format(self.verbosity),
             self.test_id,
-            self.test_options_flags,
+        ] + self.test_options_flags + [
             '--settings=test',
             self.extra_args,
             '--with-xunitmp',
@@ -168,7 +166,7 @@ class SystemTestSuite(NoseTestSuite):
 
         cmd.extend(self.passthrough_options)
 
-        return self._under_coverage_cmd(" ".join(cmd))
+        return self._under_coverage_cmd(cmd)
 
     @property
     def _default_test_id(self):
@@ -214,19 +212,15 @@ class LibTestSuite(NoseTestSuite):
 
     @property
     def cmd(self):
-        cmd = (
-            "nosetests --id-file={test_ids} {test_id} {test_opts} "
-            "--with-xunit --xunit-file={xunit_report} {extra} "
-            "--verbosity={verbosity} "
-            "{passthrough}".format(
-                test_ids=self.test_ids,
-                test_id=self.test_id,
-                test_opts=self.test_options_flags,
-                xunit_report=self.xunit_report,
-                verbosity=self.verbosity,
-                extra=self.extra_args,
-                passthrough=self.passthrough_options
-            )
-        )
+        cmd = [
+            "nosetests",
+            "--id-file={}".format(self.test_ids),
+            self.test_id,
+        ] + self.test_options_flags + [
+            "--with-xunit",
+            "--xunit-file={}".format(self.xunit_report),
+            self.extra_args,
+            "--verbosity={}".format(self.verbosity),
+        ] + self.passthrough_options
 
         return self._under_coverage_cmd(cmd)

pavelib/utils/test/suites/suite.py

@@ -76,7 +76,7 @@ class TestSuite(object):
         It returns False if errors or failures occur. Otherwise, it
         returns True.
         """
-        cmd = self.cmd
+        cmd = " ".join(self.cmd)
 
         if tasks.environment.dry_run:
             tasks.environment.info(cmd)