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

samdoran · 2021-09-08T16:00:14Z

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

This will resolve the test failures in #805.

test/unit/config/test__base.py

pytest.ini

Shrews

Haven't looked into why the tests are failing, but I'm on board with all of these changes. Love the s/tmpdir/tmp_path/ part.

test/unit/config/test_doc.py

pytest.ini

Shrews · 2021-09-10T19:55:10Z

recheck

That SKIPPED zuul job is weird.

Shrews · 2021-09-10T19:56:11Z

recheck

samdoran · 2021-09-10T20:07:40Z

There are some test failures I need to work out. I can't run the integration tests locally, so I just have to kick them up and see what fails. 🤞

samdoran · 2021-09-11T01:35:45Z

I think I've fixed the test failures related to switch to tmp_path. It's a pathlib.Path object so the mkdir() method has a slightly different behavior.

samdoran · 2021-09-11T02:18:30Z

Latest failure is because pathlib.Path.readlink() is Python >= 3.9 only and I only tested in Python 3.9 locally. Hopefully that's the last failure.

samdoran · 2021-09-11T05:23:58Z

These tests make me sad. :(

AlanCoding · 2021-09-12T01:47:40Z

You can do it! ansible-runner needed a hero!

pytest.ini

AlanCoding · 2021-09-13T02:59:45Z

Checking out your branch and running locally, here's the failure I get (for a lot of tests):

https://gist.github.com/AlanCoding/15377a3099068506a6bfdb05b9b740d4

So there's still some brittleness somehow.

It confusingly gives the help text. Looking into the code path, I printed out sys.argv (right inside the main method) with and without multiprocessing turned on.

off: ['/home/alancoding/repos/awx/env/bin/py.test', 'test/integration/test_main.py', '-k', 'test_role_logfile_abs']
on: ['-c']

And it turns out that sys.argv is used to make decisions, so my failures with multiprocessing on come down to this bit of code.

ansible-runner/ansible_runner/__main__.py

Lines 757 to 760 in 77f15a4

    
           if len(sys.argv) == 1: 
        
               parser.print_usage() 
        
               print_common_usage() 
        
               parser.exit(status=0)

Tests are ran with the pytest sys.argv (which are different for the parallel tests), and not the intended sys_args kwarg that the test is trying to pass.

The only coherent way I can see out of this is

diff --git a/ansible_runner/__main__.py b/ansible_runner/__main__.py
index d6222a1..fe2640c 100644
--- a/ansible_runner/__main__.py
+++ b/ansible_runner/__main__.py
@@ -754,7 +754,7 @@ def main(sys_args=None):
     add_args_to_parser(isalive_container_group, DEFAULT_CLI_ARGS['container_group'])
     add_args_to_parser(transmit_container_group, DEFAULT_CLI_ARGS['container_group'])
 
-    if len(sys.argv) == 1:
+    if sys_args is None:
         parser.print_usage()
         print_common_usage()
         parser.exit(status=0)

I don't know why Zuul was not hitting the same failures as me, but maybe it's because all tests run on gw0, suggesting multiprocessing scaling automatically to 1 process... but that's wild speculation.

samdoran · 2021-09-13T13:17:29Z

Yup, I ran into this reliance on sys.argv before. I ended up mocking it in the tests to get it passing. I wasn't sure if that was a problem with the tests or if the code should be changed.

samdoran · 2021-09-13T13:26:51Z

I don't know why Zuul was not hitting the same failures as me, but maybe it's because all tests run on gw0, suggesting multiprocessing scaling automatically to 1 process... but that's wild speculation.

It does seem to me that the tests are passing by accident currently. Not a great state of affairs.

AlanCoding · 2021-09-13T13:48:14Z

Yup, I ran into this reliance on sys.argv before. I ended up mocking it in the tests to get it passing. I wasn't sure if that was a problem with the tests or if the code should be changed.

I find the code's use of sys.argv there to be indefensible. It looks like a programming error. If it's easy for you to incorporate it, then go ahead, otherwise I would like to return to it and remove such mocks in a followup PR. You've already got a lot of good work here, and I'd like to do whatever I can to keep things moving forward.

webknjaz · 2021-09-13T14:17:44Z

test/unit/config/test__base.py

-        rc._prepare_env()
-        rc._handle_command_wrap(rc.execution_mode, rc.cmdline_args)
+def test_containerization_settings(tmp_path, container_runtime, mocker):
+    mocker.patch.dict('os.environ', {'HOME': str(tmp_path)}, clear=True)


Have you considered using the built-in monkeypatch.setenv()?

https://docs.pytest.org/en/latest/how-to/monkeypatch.html

I haven't yet figured out when to use monkeypatch vs mocker.patch. I still have things to learn. :)

out of the 3 options outlined here:

pytest-dev/pytest#4576 (comment)

I have seen a lot converging on option (2), because python2 tends to be in the rear view mirror by now. The pattern from unittest import mock already exists in ansible-runner unit tests. However, the pytest-mock you're adding here is more naturally aware of the pytest test scope, which means you don't have to indent everything in context managers. Thus, I raise no objections to it.

The pattern from unittest import mock already exists in ansible-runner unit tests.

I have a stashed commit locally that removes all of that in favor of the pytest-mock plugin. It ended up being pretty huge and took me several hours to get working which is why I didn't include those changes in this PR. I'll do that in (yet another) PR.

I had a good talk with @webknjaz about monkeypach vs mocker.patch and I have a better idea of when to use each now. monkeypatch doesn't have the ability to clear the environment, but I'm not sure that is strictly necessary. I will try it out see if it works.

Technically, monkeypatch.setattr(os, 'environ', {'HOME': str(tmp_path)}) would probably emulate this. But I agree, it's usually unnecessary to clear all the other unnecessary env vars. If you expect certain env vars not to be set, use monkeypatch.delenv() for that.

out of the 3 options outlined here:

pytest-dev/pytest#4576 (comment)

I think that what Ronny points out in the very next comment is even more important to take into account: #815 (comment).

TL;DR If you end up using any sort of mocking technique, you probably have an architectural problem. If you were to just use dependency injection, there would be exactly zero need for this.

If you end up using any sort of mocking technique, you probably have an architectural problem. If you were to just use dependency injection, there would be exactly zero need for this.

I absolutely agree with this. I am huge fan of test driven development for this reason, among others. Currently, I'm just trying to improve the state of the tests. Eventually I hope to improve the code itself.

samdoran · 2021-09-13T20:35:26Z

The sys.argv docs provide an explanation of where the -c comes from:

If the command was executed using the -c command line option to the interpreter, argv[0] is set to the string '-c'.

pytest-xdist obscures the original command line args in sys.argv.

webknjaz · 2021-09-13T21:18:32Z

Oh, if you only new what hacks CPython does with -c and -m 🤯

webknjaz · 2021-09-13T21:19:20Z

FWIW I believe it's wrong to rely on argv in tests. One should fall an entrypoint function where it's passed in as an arg.

AlanCoding · 2021-09-14T01:29:52Z

Yes, I only highlighted the sys.argv to illustrate that they were pytest args, as opposed to ansible-runner args. The code does ogles of processing of args but only directly references sys.argv in one suspiciously trivial use. This was a bug, and writing tests to accommodate the bug is not ideal.

webknjaz · 2021-09-14T12:06:20Z

This was a bug, and writing tests to accommodate the bug is not ideal.

This is a bug in ansible-runner (and its tests), though.

AlanCoding · 2021-09-14T13:15:12Z

I still believe this diff would fix the bug, sorry if I haven't communicated very clearly.

samdoran · 2021-09-14T16:13:36Z

@AlanCoding Unfortunately, it doesn't. I spent all day yesterday and all morning on a fix. It's quite a bit more complicated than I had hoped, though I'm hoping we can go with a much simpler fix than what I came up with in order to keep the existing behavior.

pathlib.Path.mkdir() returns None, not a Path object.

Also make sure to write temp files to tmp_path and not in the project root.

This reverts commit 15be299.

A lot of these tests are brittle because they are writing files to the project directory and checking for the existence of files in the same directory. Using a different directory for each test makes this more resilient.

samdoran · 2021-09-14T21:58:56Z

Ho-lee-cow. It passed. 🙌

ansible-zuul

LGTM!

Do not rely on length of sys.argv From the current behavior, it seems the goal is to print a common usage message and exit 0 if no sub command was given. Under other circumstances, such as an invalid or missing parameter, exit with 2 and print the normal usage message. In order to preserve this behavior and make the tests run consistently whether or not they are run in multiple process, a custom error method was created that inspects the error message and exits 0 with the common usage message if no command was given. This is a bit fragile, but inside that method we do not have easy access to arguments passed to the main() function (they could be inferred by inspecting private attributes, but this is probably unwise). This requires catching the SystemExit in the main() function and deciding there based on sys_args how to handle it. This fix preservers the existing behavior whil making the tests pass consistently. However, it would be much simpler to rely on the default behavior of argparse and exit 2 in both cases. This would remove the need for a custom error method and exception handling. It would be a change in behavior, though. Related to #815. Reviewed-by: Alan Rominger <arominge@redhat.com> Reviewed-by: Sam Doran <sdoran@redhat.com> Reviewed-by: David Shrewsbury <None> Reviewed-by: None <None>

Do not rely on length of sys.argv From the current behavior, it seems the goal is to print a common usage message and exit 0 if no sub command was given. Under other circumstances, such as an invalid or missing parameter, exit with 2 and print the normal usage message. In order to preserve this behavior and make the tests run consistently whether or not they are run in multiple process, a custom error method was created that inspects the error message and exits 0 with the common usage message if no command was given. This is a bit fragile, but inside that method we do not have easy access to arguments passed to the main() function (they could be inferred by inspecting private attributes, but this is probably unwise). This requires catching the SystemExit in the main() function and deciding there based on sys_args how to handle it. This fix preservers the existing behavior whil making the tests pass consistently. However, it would be much simpler to rely on the default behavior of argparse and exit 2 in both cases. This would remove the need for a custom error method and exception handling. It would be a change in behavior, though. Related to #815. Reviewed-by: Alan Rominger <arominge@redhat.com> Reviewed-by: Sam Doran <sdoran@redhat.com> Reviewed-by: David Shrewsbury <None> Reviewed-by: None <None> (cherry picked from commit 4f67a3a)

…4f67a3a9e2f20e5d4da313958881eeef67397011/pr-825 [PR #825/4f67a3a9 backport][release_2.0] Do not rely on length of sys.argv This is a backport of PR #825 as merged into devel (4f67a3a). From the current behavior, it seems the goal is to print a common usage message and exit 0 if no sub command was given. Under other circumstances, such as an invalid or missing parameter, exit with 2 and print the normal usage message. In order to preserve this behavior and make the tests run consistently whether or not they are run in multiple process, a custom error method was created that inspects the error message and exits 0 with the common usage message if no command was given. This is a bit fragile, but inside that method we do not have easy access to arguments passed to the main() function (they could be inferred by inspecting private attributes, but this is probably unwise). This requires catching the SystemExit in the main() function and deciding there based on sys_args how to handle it. This fix preservers the existing behavior whil making the tests pass consistently. However, it would be much simpler to rely on the default behavior of argparse and exit 2 in both cases. This would remove the need for a custom error method and exception handling. It would be a change in behavior, though. Related to #815. Reviewed-by: David Shrewsbury <None> Reviewed-by: None <None>

patchback · 2021-10-14T15:47:58Z

Backport to release_2.0: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 2323686 on top of patchback/backports/release_2.0/2323686deac3a0e6691b7bd63c156f8422529746/pr-815

Backporting merged PR #815 into devel

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/ansible/ansible-runner.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/release_2.0/2323686deac3a0e6691b7bd63c156f8422529746/pr-815 upstream/release_2.0

Now, cherry-pick PR Fix unit tests to run consistently in CI and locally #815 contents into that branch:
```
$ git cherry-pick -x 2323686deac3a0e6691b7bd63c156f8422529746
```
If it'll yell at you with something like fatal: Commit 2323686deac3a0e6691b7bd63c156f8422529746 is a merge but no -m option was given., add -m 1 as follows intead:
```
$ git cherry-pick -m1 -x 2323686deac3a0e6691b7bd63c156f8422529746
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix unit tests to run consistently in CI and locally #815 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/release_2.0/2323686deac3a0e6691b7bd63c156f8422529746/pr-815

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

Fix unit tests to run consistently in CI and locally 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 This will resolve the test failures in ansible#805. Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com> Reviewed-by: Sam Doran <sdoran@redhat.com> Reviewed-by: Alan Rominger <arominge@redhat.com> Reviewed-by: David Shrewsbury <None> Reviewed-by: None <None> (cherry picked from commit 2323686)

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

[release-2.0] Backport test fixes PR #772 PR #815 PR #831 PR #838 PR #859 Reviewed-by: Shane McDonald <me@shanemcd.com> Reviewed-by: None <None>

samdoran requested a review from a team as a code owner September 8, 2021 16:00

webknjaz reviewed Sep 8, 2021

View reviewed changes

test/unit/config/test__base.py Outdated Show resolved Hide resolved

samdoran closed this Sep 9, 2021

This was referenced Sep 9, 2021

Try to make another test robust to run on other OSes #819

Closed

Cleanup test file #818

Closed

samdoran reopened this Sep 10, 2021

webknjaz reviewed Sep 10, 2021

View reviewed changes

pytest.ini Show resolved Hide resolved

samdoran force-pushed the ci/isolate-unit-tests branch from 6ca381e to a37d474 Compare September 10, 2021 16:28

AlanCoding approved these changes Sep 10, 2021

View reviewed changes

Shrews reviewed Sep 10, 2021

View reviewed changes

test/unit/config/test_doc.py Show resolved Hide resolved

pytest.ini Show resolved Hide resolved

AlanCoding reviewed Sep 12, 2021

View reviewed changes

pytest.ini Show resolved Hide resolved

webknjaz reviewed Sep 13, 2021

View reviewed changes

samdoran added 11 commits September 14, 2021 17:29

Update test requirements

8960e5a

Add descriptions to tox environments

2b3c3d2

Switch to using tmp_path fixture

3e4b254

Incorporate fixes from Alan

a0f5641

Integration test fixes

2d499ac

pathlib.Path.mkdir() returns None, not a Path object.

Use os.readlink() since Python <3.9 does not have Path.readlink()

d8d17d0

One more

2aab3d9

Cleanup some homegrown tempfile management

260a518

Also make sure to write temp files to tmp_path and not in the project root.

Revert "Cleanup some homegrown tempfile management"

be10ee4

This reverts commit 15be299.

Use unique temporary directories

0ff39bb

A lot of these tests are brittle because they are writing files to the project directory and checking for the existence of files in the same directory. Using a different directory for each test makes this more resilient.

Separate interfering tests

041fa40

samdoran force-pushed the ci/isolate-unit-tests branch from ff46f7a to 041fa40 Compare September 14, 2021 21:31

samdoran added the gate label Sep 14, 2021

ansible-zuul bot approved these changes Sep 14, 2021

View reviewed changes

Shrews approved these changes Sep 14, 2021

View reviewed changes

ansible-zuul bot merged commit 2323686 into ansible:devel Sep 14, 2021

samdoran mentioned this pull request Sep 15, 2021

Run tests using GitHub Actions ansible/ansible-builder#284

Merged

samdoran deleted the ci/isolate-unit-tests branch September 20, 2021 20:44

patchback bot mentioned this pull request Sep 21, 2021

[PR #825/4f67a3a9 backport][release_2.0] Do not rely on length of sys.argv #842

Merged

samdoran added the backport-2.0 label Oct 14, 2021

samdoran mentioned this pull request Oct 14, 2021

[release_2.0] Fix unit tests to run consistently in CI and locally (#815) #869

Closed

samdoran mentioned this pull request Oct 14, 2021

[release-2.0] Backport test fixes #871

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

samdoran commented Sep 8, 2021 •

edited

Shrews left a comment

Shrews commented Sep 10, 2021

Shrews commented Sep 10, 2021

samdoran commented Sep 10, 2021

samdoran commented Sep 11, 2021

samdoran commented Sep 11, 2021

samdoran commented Sep 11, 2021

AlanCoding commented Sep 12, 2021

AlanCoding commented Sep 13, 2021

samdoran commented Sep 13, 2021 •

edited

samdoran commented Sep 13, 2021

AlanCoding commented Sep 13, 2021

webknjaz Sep 13, 2021

samdoran Sep 13, 2021 •

edited

AlanCoding Sep 13, 2021

samdoran Sep 13, 2021

samdoran Sep 13, 2021

webknjaz Sep 13, 2021

webknjaz Sep 13, 2021

samdoran Sep 14, 2021

samdoran commented Sep 13, 2021

webknjaz commented Sep 13, 2021

webknjaz commented Sep 13, 2021

AlanCoding commented Sep 14, 2021

webknjaz commented Sep 14, 2021 •

edited

AlanCoding commented Sep 14, 2021

samdoran commented Sep 14, 2021

samdoran commented Sep 14, 2021

ansible-zuul bot left a comment

patchback bot commented Oct 14, 2021 •

edited

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

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

Conversation

samdoran commented Sep 8, 2021 • edited

Shrews left a comment

Choose a reason for hiding this comment

Shrews commented Sep 10, 2021

Shrews commented Sep 10, 2021

samdoran commented Sep 10, 2021

samdoran commented Sep 11, 2021

samdoran commented Sep 11, 2021

samdoran commented Sep 11, 2021

AlanCoding commented Sep 12, 2021

AlanCoding commented Sep 13, 2021

samdoran commented Sep 13, 2021 • edited

samdoran commented Sep 13, 2021

AlanCoding commented Sep 13, 2021

webknjaz Sep 13, 2021

Choose a reason for hiding this comment

samdoran Sep 13, 2021 • edited

Choose a reason for hiding this comment

AlanCoding Sep 13, 2021

Choose a reason for hiding this comment

samdoran Sep 13, 2021

Choose a reason for hiding this comment

samdoran Sep 13, 2021

Choose a reason for hiding this comment

webknjaz Sep 13, 2021

Choose a reason for hiding this comment

webknjaz Sep 13, 2021

Choose a reason for hiding this comment

samdoran Sep 14, 2021

Choose a reason for hiding this comment

samdoran commented Sep 13, 2021

webknjaz commented Sep 13, 2021

webknjaz commented Sep 13, 2021

AlanCoding commented Sep 14, 2021

webknjaz commented Sep 14, 2021 • edited

AlanCoding commented Sep 14, 2021

samdoran commented Sep 14, 2021

samdoran commented Sep 14, 2021

ansible-zuul bot left a comment

Choose a reason for hiding this comment

patchback bot commented Oct 14, 2021 • edited

Backport to release_2.0: 💔 cherry-picking failed — conflicts found

Backporting merged PR #815 into devel

samdoran commented Sep 8, 2021 •

edited

samdoran commented Sep 13, 2021 •

edited

samdoran Sep 13, 2021 •

edited

webknjaz commented Sep 14, 2021 •

edited

patchback bot commented Oct 14, 2021 •

edited