Fix unit tests to run consistently in CI and locally (ansible#815)

Use pytest-mock and tmp_path fixtures
Mock test environment variables to prevent inconsistent test results
Reformat test fixtures so they are easier to read

test/integration/conftest.py

@@ -7,14 +7,14 @@
 
 
 @pytest.fixture(scope='function')
-def rc(tmpdir):
-    rc = RunnerConfig(str(tmpdir))
+def rc(tmp_path):
+    rc = RunnerConfig(str(tmp_path))
     rc.suppress_ansible_output = True
     rc.expect_passwords = {
         pexpect.TIMEOUT: None,
         pexpect.EOF: None
     }
-    rc.cwd = str(tmpdir)
+    rc.cwd = str(tmp_path)
     rc.env = {}
     rc.job_timeout = 10
     rc.idle_timeout = 0

test/integration/test_display_callback.py

@@ -13,8 +13,9 @@
 
 
 @pytest.fixture()
-def executor(tmpdir, request, is_pre_ansible28):
-    private_data_dir = six.text_type(tmpdir.mkdir('foo'))
+def executor(tmp_path, request, is_pre_ansible28):
+    private_data_dir = tmp_path / 'foo'
+    private_data_dir.mkdir()
 
     playbooks = request.node.callspec.params.get('playbook')
     playbook = list(playbooks.values())[0]
@@ -317,7 +318,7 @@ def test_module_level_no_log(executor, playbook, skipif_pre_ansible28):
     assert 'SENSITIVE' not in json.dumps(list(executor.events))
 
 
-def test_output_when_given_invalid_playbook(tmpdir):
+def test_output_when_given_invalid_playbook(tmp_path):
     # As shown in the following issue:
     #
     #   https://github.com/ansible/ansible-runner/issues/29
@@ -328,7 +329,7 @@ def test_output_when_given_invalid_playbook(tmpdir):
     #   https://github.com/ansible/ansible-runner/pull/34
     #
     # But no test validated it.  This does that.
-    private_data_dir = str(tmpdir)
+    private_data_dir = str(tmp_path)
     executor = init_runner(
         private_data_dir=private_data_dir,
         inventory="localhost ansible_connection=local",
@@ -342,7 +343,7 @@ def test_output_when_given_invalid_playbook(tmpdir):
     assert "could not be found" in stdout
 
 
-def test_output_when_given_non_playbook_script(tmpdir):
+def test_output_when_given_non_playbook_script(tmp_path):
     # As shown in the following pull request:
     #
     #   https://github.com/ansible/ansible-runner/pull/256
@@ -354,7 +355,7 @@ def test_output_when_given_non_playbook_script(tmpdir):
     # this is a retro-active test based on the sample repo provided in the PR:
     #
     #   https://github.com/AlanCoding/ansible-runner-examples/tree/master/non_playbook/sleep_with_writes
-    private_data_dir = str(tmpdir)
+    private_data_dir = str(tmp_path)
     with open(os.path.join(private_data_dir, "args"), 'w') as args_file:
         args_file.write("bash sleep_and_write.sh\n")
     with open(os.path.join(private_data_dir, "sleep_and_write.sh"), 'w') as script_file:

test/integration/test_main.py

@@ -124,11 +124,10 @@ def test_module_run_debug(tmp_path):
     assert rc == 0
 
 
-def test_module_run_clean():
-    with temp_directory() as temp_dir:
-        rc = main(['run', '-m', 'ping',
-                   '--hosts', 'localhost',
-                   temp_dir])
+def test_module_run_clean(tmp_path):
+    rc = main(['run', '-m', 'ping',
+               '--hosts', 'localhost',
+               str(tmp_path)])
     assert rc == 0
 
 
@@ -155,6 +154,9 @@ def test_role_run_abs(tmp_path):
     assert rc == 0
 
 
+# FIXME: This test interferes with test_role_logfile_abs. Marking it as serial so it is executed
+#        in a separate test run.
+@pytest.mark.serial
 def test_role_logfile(skipif_pre_ansible28, tmp_path):
     logfile = tmp_path.joinpath('test_role_logfile')
     rc = main(['run', '-r', 'benthomasson.hello_role',
@@ -167,6 +169,17 @@ def test_role_logfile(skipif_pre_ansible28, tmp_path):
     assert rc == 0
 
 
+def test_role_logfile_abs(tmp_path):
+    rc = main(['run', '-r', 'benthomasson.hello_role',
+               '--hosts', 'localhost',
+               '--roles-path', os.path.join(HERE, 'project/roles'),
+               '--logfile', tmp_path.joinpath('new_logfile').as_posix(),
+               str(tmp_path)])
+
+    assert tmp_path.joinpath('new_logfile').exists()
+    assert rc == 0
+
+
 def test_role_bad_project_dir():
 
     with open("bad_project_dir", 'w') as f:

test/integration/test_transmit_worker_process.py

@@ -53,10 +53,14 @@ def check_artifacts(self, process_dir, job_type):
             assert '"ansible_facts"' in stdout
 
     @pytest.mark.parametrize("job_type", ['run', 'adhoc'])
-    def test_remote_job_interface(self, tmpdir, test_data_dir, job_type):
+    def test_remote_job_interface(self, tmp_path, test_data_dir, job_type):
         transmit_dir = os.path.join(test_data_dir, 'debug')
-        worker_dir = str(tmpdir.mkdir('for_worker'))
-        process_dir = str(tmpdir.mkdir('for_process'))
+        worker_dir = tmp_path / 'for_worker'
+        worker_dir.mkdir()
+
+        process_dir = tmp_path / 'for_process'
+        process_dir.mkdir()
+
         job_kwargs = self.get_job_kwargs(job_type)
 
         outgoing_buffer = tempfile.NamedTemporaryFile()
@@ -90,17 +94,21 @@ def test_remote_job_interface(self, tmpdir, test_data_dir, job_type):
         processor = Processor(_input=incoming_buffer, private_data_dir=process_dir)
         processor.run()
 
-        self.check_artifacts(process_dir, job_type)
+        self.check_artifacts(str(process_dir), job_type)
 
     @pytest.mark.parametrize("job_type", ['run', 'adhoc'])
-    def test_remote_job_by_sockets(self, tmpdir, test_data_dir, container_runtime_installed, job_type):
+    def test_remote_job_by_sockets(self, tmp_path, test_data_dir, container_runtime_installed, job_type):
         """This test case is intended to be close to how the AWX use case works
         the process interacts with receptorctl with sockets
         sockets are used here, but worker is manually called instead of invoked by receptor
         """
         transmit_dir = os.path.join(test_data_dir, 'debug')
-        worker_dir = str(tmpdir.mkdir('for_worker'))
-        process_dir = str(tmpdir.mkdir('for_process'))
+        worker_dir = tmp_path / 'for_worker'
+        worker_dir.mkdir()
+
+        process_dir = tmp_path / 'for_process'
+        process_dir.mkdir()
+
         job_kwargs = self.get_job_kwargs(job_type)
 
         def transmit_method(transmit_sockfile_write):
@@ -155,7 +163,7 @@ def process_method(results_sockfile_read):
 
         assert set(os.listdir(worker_dir)) == {'artifacts', 'inventory', 'project', 'env'}
 
-        self.check_artifacts(process_dir, job_type)
+        self.check_artifacts(str(process_dir), job_type)
 
 
 def test_missing_private_dir_transmit(tmpdir):
@@ -173,8 +181,9 @@ def test_missing_private_dir_transmit(tmpdir):
     assert "private_data_dir path is either invalid or does not exist" in str(excinfo.value)
 
 
-def test_garbage_private_dir_worker(tmpdir):
-    worker_dir = str(tmpdir.mkdir('for_worker'))
+def test_garbage_private_dir_worker(tmp_path):
+    worker_dir = tmp_path / 'for_worker'
+    worker_dir.mkdir()
     incoming_buffer = io.BytesIO(
         b'{"kwargs": {"playbook": "debug.yml"}}\n{"zipfile": 5}\n\x01\x02\x03\x04\x05{"eof": true}\n')
     outgoing_buffer = io.BytesIO()
@@ -190,8 +199,9 @@ def test_garbage_private_dir_worker(tmpdir):
     assert b'"status": "error"' in sent
 
 
-def test_unparsable_private_dir_worker(tmpdir):
-    worker_dir = str(tmpdir.mkdir('for_worker'))
+def test_unparsable_private_dir_worker(tmp_path):
+    worker_dir = tmp_path / 'for_worker'
+    worker_dir.mkdir()
     incoming_buffer = io.BytesIO(b'')
     outgoing_buffer = io.BytesIO()
 
@@ -206,8 +216,9 @@ def test_unparsable_private_dir_worker(tmpdir):
     assert b'"status": "error"' in sent
 
 
-def test_unparsable_private_dir_processor(tmpdir):
-    process_dir = str(tmpdir.mkdir('for_process'))
+def test_unparsable_private_dir_processor(tmp_path):
+    process_dir = tmp_path / 'for_process'
+    process_dir.mkdir()
     incoming_buffer = io.BytesIO(b'')
 
     processor = run(

test/unit/config/test__base.py

@@ -280,7 +280,7 @@ def test_container_volume_mounting_with_Z(tmp_path, mocker):
 
 
 @pytest.mark.parametrize('container_runtime', ['docker', 'podman'])
-def test_containerization_settings(tmpdir, container_runtime):
+def test_containerization_settings(tmp_path, container_runtime, mocker):
     mocker.patch.dict('os.environ', {'HOME': str(tmp_path)}, clear=True)
     tmp_path.joinpath('.ssh').mkdir()
 
@@ -317,10 +317,8 @@ def test_containerization_settings(tmpdir, container_runtime):
         '-v', '{}/.ssh/:/home/runner/.ssh/'.format(str(tmp_path)),
     ]
 
-    expected_command_start = [container_runtime, 'run', '--rm', '--tty', '--interactive', '--workdir', '/runner/project'] + \
-                             ['-v', '{}/.ssh/:/home/runner/.ssh/'.format(os.environ['HOME'])]
     if container_runtime == 'podman':
-        expected_command_start += ['--group-add=root', '--ipc=host']
+        expected_command_start.extend(['--group-add=root', '--ipc=host'])
 
     expected_command_start.extend([
         '-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir),
@@ -330,19 +328,12 @@ def test_containerization_settings(tmpdir, container_runtime):
 
     expected_command_start.extend(extra_container_args)
 
-    expected_command_start += ['-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir)] + \
-        ['-v', '{}/:/runner/:Z'.format(rc.private_data_dir)] + \
-        ['--env-file', '{}/env.list'.format(rc.artifact_dir)] + \
-        extra_container_args + \
-        ['--name', 'ansible_runner_foo'] + \
-        ['my_container', 'ansible-playbook', 'main.yaml', '-i', '/tmp/inventory']
+    expected_command_start.extend([
+        '--name', 'ansible_runner_foo',
+        'my_container', 'ansible-playbook', 'main.yaml', '-i', '/tmp/inventory',
     ])
 
-    for index, element in enumerate(expected_command_start):
-        if '--user=' in element:
-            assert '--user=' in rc.command[index]
-        else:
-            assert rc.command[index] == element
+    assert expected_command_start == rc.command
 
 
 @pytest.mark.parametrize(

test/unit/config/test_ansible_cfg.py

@@ -55,9 +55,12 @@ def test_prepare_config_invalid_action():
 
 
 @pytest.mark.parametrize('container_runtime', ['docker', 'podman'])
-def test_prepare_config_command_with_containerization(tmpdir, container_runtime):
+def test_prepare_config_command_with_containerization(tmp_path, container_runtime, mocker):
+    mocker.patch.dict('os.environ', {'HOME': str(tmp_path)}, clear=True)
+    tmp_path.joinpath('.ssh').mkdir()
+
     kwargs = {
-        'private_data_dir': tmpdir,
+        'private_data_dir': tmp_path,
         'process_isolation': True,
         'container_image': 'my_container',
         'process_isolation_executable': container_runtime
@@ -71,22 +74,37 @@ def test_prepare_config_command_with_containerization(tmpdir, container_runtime)
     if container_runtime == 'podman':
         extra_container_args = ['--quiet']
     else:
-        extra_container_args = ['--user={os.getuid()}']
+        extra_container_args = [f'--user={os.getuid()}']
+
+    expected_command_start = [
+        container_runtime,
+        'run',
+        '--rm',
+        '--interactive',
+        '--workdir',
+        '/runner/project',
+        '-v', '{}/.ssh/:/home/runner/.ssh/'.format(rc.private_data_dir),
+    ]
 
-    expected_command_start = [container_runtime, 'run', '--rm', '--interactive', '--workdir', '/runner/project'] + \
-                             ['-v', '{}/.ssh/:/home/runner/.ssh/'.format(os.environ['HOME'])]
     if container_runtime == 'podman':
-        expected_command_start += ['--group-add=root', '--ipc=host']
-
-    expected_command_start += ['-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir)] + \
-        ['-v', '{}/:/runner/:Z'.format(rc.private_data_dir)] + \
-        ['--env-file', '{}/env.list'.format(rc.artifact_dir)] + \
-        extra_container_args + \
-        ['--name', 'ansible_runner_foo'] + \
-        ['my_container', 'ansible-config', 'list', '-c', '/tmp/ansible.cfg']
-
-    for index, element in enumerate(expected_command_start):
-        if '--user=' in element:
-            assert '--user=' in rc.command[index]
-        else:
-            assert rc.command[index] == element
+        expected_command_start.extend(['--group-add=root', '--ipc=host'])
+
+    expected_command_start.extend([
+        '-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir),
+        '-v', '{}/:/runner/:Z'.format(rc.private_data_dir),
+        '--env-file', '{}/env.list'.format(rc.artifact_dir),
+    ])
+
+    expected_command_start.extend(extra_container_args)
+
+    expected_command_start.extend([
+        '--name',
+        'ansible_runner_foo',
+        'my_container',
+        'ansible-config',
+        'list',
+        '-c',
+        '/tmp/ansible.cfg',
+    ])
+
+    assert expected_command_start == rc.command

test/unit/config/test_command.py

@@ -64,9 +64,12 @@ def test_prepare_run_command_generic():
 
 
 @pytest.mark.parametrize('container_runtime', ['docker', 'podman'])
-def test_prepare_run_command_with_containerization(tmpdir, container_runtime):
+def test_prepare_run_command_with_containerization(tmp_path, container_runtime, mocker):
+    mocker.patch.dict('os.environ', {'HOME': str(tmp_path)}, clear=True)
+    tmp_path.joinpath('.ssh').mkdir()
+
     kwargs = {
-        'private_data_dir': tmpdir,
+        'private_data_dir': tmp_path,
         'process_isolation': True,
         'container_image': 'my_container',
         'process_isolation_executable': container_runtime
@@ -83,23 +86,38 @@ def test_prepare_run_command_with_containerization(tmpdir, container_runtime):
     if container_runtime == 'podman':
         extra_container_args = ['--quiet']
     else:
-        extra_container_args = ['--user={os.getuid()}']
-
-    expected_command_start = [container_runtime, 'run', '--rm', '--tty', '--interactive', '--workdir', '/runner/project'] + \
-                             ['-v', '{}/:{}/'.format(cwd, cwd), '-v', '{}/.ssh/:/home/runner/.ssh/'.format(os.environ['HOME'])]
+        extra_container_args = [f'--user={os.getuid()}']
+
+    expected_command_start = [
+        container_runtime,
+        'run',
+        '--rm',
+        '--tty',
+        '--interactive',
+        '--workdir',
+        '/runner/project',
+        '-v', '{}/:{}/'.format(cwd, cwd),
+        '-v', '{}/.ssh/:/home/runner/.ssh/'.format(rc.private_data_dir),
+    ]
 
     if container_runtime == 'podman':
-        expected_command_start += ['--group-add=root', '--ipc=host']
-
-    expected_command_start += ['-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir)] + \
-        ['-v', '{}/:/runner/:Z'.format(rc.private_data_dir)] + \
-        ['--env-file', '{}/env.list'.format(rc.artifact_dir)] + \
-        extra_container_args + \
-        ['--name', 'ansible_runner_foo'] + \
-        ['my_container'] + [executable_cmd] + cmdline_args
-
-    for index, element in enumerate(expected_command_start):
-        if '--user=' in element:
-            assert '--user=' in rc.command[index]
-        else:
-            assert rc.command[index] == element
+        expected_command_start.extend(['--group-add=root', '--ipc=host'])
+
+    expected_command_start.extend([
+        '-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir),
+        '-v', '{}/:/runner/:Z'.format(rc.private_data_dir),
+        '--env-file', '{}/env.list'.format(rc.artifact_dir),
+    ])
+
+    expected_command_start.extend(extra_container_args)
+
+    expected_command_start.extend([
+        '--name',
+        'ansible_runner_foo',
+        'my_container',
+        executable_cmd,
+    ])
+
+    expected_command_start.extend(cmdline_args)
+
+    assert expected_command_start == rc.command