Migrate some test to pytest #740

cans · 2020-04-19T13:59:21Z

Contributes to #725 but does not pretend to be a complete migration.

Those changes are made to factor in codes that will be impacted by fixing issue #739.

test/TestImportIncludeRole.py

test/conftest.py

test/TestTaskIncludes.py

test/TestRulesCollection.py

webknjaz · 2020-04-20T14:46:49Z

@cans I've added some pytest linters so this needs rebasing

cans · 2020-04-20T20:12:51Z

@webknjaz I don't know how your CI is setup, but I never understood how to ask pre-commit to install flake8 plugins (I'd be happy to know if you have/find a way), and they indeed did not get installed on my machine. Hence the checks you run on your CI are stricter than those pre-commit will run locally, resulting in failing tests (e.g. https://github.com/ansible/ansible-lint/pull/740/checks?check_run_id=603029144)

This is a bit annoying.

cans · 2020-04-20T20:56:02Z

Tests failed 'cause of a network error:

Collecting ansible@ https://toshio.fedorapeople.org/ansible/acd/ansible/ansible-2.10.0.tar.gz
[...]
ERROR: Could not install packages due to an EnvironmentError: HTTPSConnectionPool(host='toshio.fedorapeople.org', port=443):
 Max retries exceeded with url: /ansible/acd/ansible/ansible-2.10.0.tar.gz
 (Caused by NewConnectionError('<pip._vendor.urllib3.connection.VerifiedHTTPSConnection object at 0x7ffae517b0b8>:
 Failed to establish a new connection: [Errno 101] Network is unreachable',))

webknjaz · 2020-04-21T17:55:22Z

I don't know how your CI is setup, but I never understood how to ask pre-commit to install flake8 plugins (I'd be happy to know if you have/find a way), and they indeed did not get installed on my machine.

pre-commit installs extra pip deps automatically, like this:
https://github.com/ansible/ansible-lint/blob/cd14978/.pre-commit-config.yaml#L42-L45. If it doesn't for you, it's a bug or you have an outdated version of it.

Are you running it via tox? tox -e lint should do the trick (that's what the CIs execute and is tested by many people under many OSs).
If it doesn't work, try rm -rf ~/.pre-commit ~/.cache/pre-commit .tox && pip install --upgrade tox virtualenv && tox -e lint and see if that fixes it.

Tests failed 'cause of a network error:

Right, GH seems to be having outages today and I saw some unicorns yesterday too. Restarted.

webknjaz · 2020-04-21T17:56:11Z

recheck

test/TestDependenciesInMeta.py

webknjaz · 2020-04-21T18:33:04Z

test/TestImportIncludeRole.py

+        assert message in str(results)
+
+
+def test_invalid_import_role(default_rules_collection, play_and_error):


It'd be easier to follow if parametrize was applied here and not hidden in the fixture implementation or maybe if there were fixtures decoupling playbook&error back into two args

test/TestPretaskIncludePlaybook.py

webknjaz · 2020-04-21T18:48:39Z

test/TestRunner.py

+def test_runner_count(default_rules_collection):
+    filename = 'test/nomatchestest.yml'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    assert (len(runner.run()) == 0)
+
+
+def test_unicode_runner_count(default_rules_collection):
+    filename = 'test/unicode.yml'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    assert (len(runner.run()) == 1)
+
+
+def test_unicode_standard_formatting(default_rules_collection):
+    filename = 'test/unicode.yml'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    matches = runner.run()
+    formatter = ansiblelint.formatters.Formatter(os.getcwd(), True)
+    formatter.format(matches[0])
+
+
+def test_unicode_parseable_colored_formatting(default_rules_collection):
+    filename = 'test/unicode.yml'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    matches = runner.run()
+    formatter = ansiblelint.formatters.ParseableFormatter(os.getcwd(), True)
+    formatter.format(matches[0], colored=True)
+
+
+def test_unicode_quiet_colored_formatting(default_rules_collection):
+    filename = 'test/unicode.yml'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    matches = runner.run()
+    formatter = ansiblelint.formatters.QuietFormatter(os.getcwd(), True)
+    formatter.format(matches[0], colored=True)
+
+
+def test_unicode_standard_color_formatting(default_rules_collection):
+    filename = 'test/unicode.yml'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    matches = runner.run()
+    formatter = ansiblelint.formatters.Formatter(os.getcwd(), True)
+    formatter.format(matches[0], colored=True)
+
+
+def test_runner_excludes_paths(default_rules_collection):
+    filename = abspath('examples/lots_of_warnings.yml', os.getcwd())
+    excludes = [filename]
+    runner = Runner(default_rules_collection, filename, [], [], excludes)
+    assert (len(runner.run()) == 0)
+
+
+def test_runner_block_count(default_rules_collection):
+    filename = 'test/block.yml'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    assert (len(runner.run()) == 0)
+
+
+def test_runner_become_count(default_rules_collection):
+    filename = 'test/become.yml'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    assert (len(runner.run()) == 0)
+
+
+def test_runner_empty_tags_count(default_rules_collection):
+    filename = 'test/emptytags.yml'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    assert (len(runner.run()) == 0)
+
+
+def test_runner_encrypted_secrets(default_rules_collection):
+    filename = 'test/contains_secrets.yml'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    assert (len(runner.run()) == 0)
+
+
+def test_dir_with_trailing_slash(default_rules_collection):
+    filename = 'test/'
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    assert (list(runner.playbooks)[0][1] == 'role')
+
+
+def test_dir_with_fullpath(default_rules_collection):
+    filename = os.path.abspath('test')
+    runner = Runner(default_rules_collection, filename, [], [], [])
+    assert (list(runner.playbooks)[0][1] == 'role')
+
+
+def test_files_not_scanned_twice(default_rules_collection):
+    checked_files = set()
+
+    filename = os.path.abspath('test/common-include-1.yml')
+    runner = Runner(default_rules_collection, filename, [], [], [], 0, checked_files)
+    run1 = runner.run()
+
+    filename = os.path.abspath('test/common-include-2.yml')
+    runner = Runner(default_rules_collection, filename, [], [], [], 0, checked_files)
+    run2 = runner.run()
+
+    assert ((len(run1) + len(run2)) == 1)


note: test in this module are also parametrizable but this can be postponed.

This is now four parametrized tests (but the last that stands on its own).

webknjaz · 2020-04-22T16:44:45Z

@cans I've figured out where the confusion with the tmpdir fixture pathlib.Path vs py.path.local is coming from: tmpdir is deprecated and is supposed to be replaced by tmp_path. As per the pytest fixtures list the suggests that there's better alternatives for tmpdir and tmpdir_factory that return old-style shims:

tmp_path
Provide a pathlib.Path object to a temporary directory which is unique to each test function.
tmp_path_factory
Make session-scoped temporary directories and return pathlib.Path objects.
tmpdir
Provide a py.path.local object to a temporary directory which is unique to each test function; replaced by tmp_path.
tmpdir_factory
Make session-scoped temporary directories and return py.path.local objects; replaced by tmp_path_factory.

I saw this in a few other places in the docs but it's not emphasized enough. I often don't remember which one to use and add tmpdir expecting pathlib(2).Path objects there when I should use tmp_path instead to achieve this. Every time I have to go to docs to find out which one is old.

/me filed a bug about it @ m-burst/flake8-pytest-style#23

test/TestSkipImportPlaybook.py

webknjaz · 2020-04-23T12:28:46Z

@cans I noticed you're trying out the co-authors feature. You used a github handle but that is an incorrect format. Compare fb88e61 (one comitter, one avatar) and 41d597d (two committers with avatars linked to both of their GH profiles).

Screenshots

The proper form would be

Co-Authored-By: Sviatoslav Sydorenko <webknjaz@redhat.com>

You can also use users' no-reply emails if they don't want to use public ones.

Ref: https://help.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors

webknjaz · 2020-04-23T12:44:05Z

test/TestImportIncludeRole.py

+def _setup_play(base_dir, playbook_text):
+    role_tasks_dir = base_dir / 'test-role' / 'tasks'
+    role_tasks_dir.mkdir(parents=True)
+    (role_tasks_dir / 'main.yml').write_text(ROLE_TASKS_MAIN)
+    (role_tasks_dir / 'world.yml').write_text(ROLE_TASKS_WORLD)
+    play_path = base_dir / 'playbook.yml'
+    play_path.write_text(playbook_text)
+    return str(play_path)
+
+
+@pytest.mark.parametrize(('playbook_filename', 'messages'), (
+    pytest.param(PLAY_IMPORT_ROLE,
+                 ['only when shell functionality is required'],
+                 id='IMPORT_ROLE',
+                 ),
+    pytest.param(PLAY_IMPORT_ROLE_INLINE,
+                 ['only when shell functionality is require'],
+                 id='IMPORT_ROLE_INLINE',
+                 ),
+    pytest.param(PLAY_INCLUDE_ROLE,
+                 ['only when shell functionality is require',
+                  'All tasks should be named'],
+                 id='INCLUDE_ROLE',
+                 ),
+    pytest.param(PLAY_INCLUDE_ROLE_INLINE,
+                 ['only when shell functionality is require',
+                  'All tasks should be named'],
+                 id='INCLUDE_ROLE_INLINE',
+                 ),
+))
+def test_import_role2(tmp_path, default_rules_collection, playbook_filename, messages):
+    playbook = _setup_play(tmp_path, playbook_filename)
+    runner = Runner(default_rules_collection, playbook, [], [], [])
+
+    results = runner.run()
+
+    for message in messages:
+        assert message in str(results)
+
+
+@pytest.mark.parametrize(('playbook_filename', 'error'), (
+    pytest.param(PLAY_IMPORT_ROLE_INCOMPLETE,
+                 "Failed to find required 'name' key in import_role",
+                 id='IMPORT_ROLE_INCOMPLETE',
+                 ),
+))
+def test_invalid_import_role(tmp_path, default_rules_collection, playbook_filename, error):
+    playbook = _setup_play(tmp_path, playbook_filename)
+    runner = Runner(default_rules_collection, playbook, [], [], [])


How about a more classical approach?

Suggested change

def _setup_play(base_dir, playbook_text):

role_tasks_dir = base_dir / 'test-role' / 'tasks'

role_tasks_dir.mkdir(parents=True)

(role_tasks_dir / 'main.yml').write_text(ROLE_TASKS_MAIN)

(role_tasks_dir / 'world.yml').write_text(ROLE_TASKS_WORLD)

play_path = base_dir / 'playbook.yml'

play_path.write_text(playbook_text)

return str(play_path)

@pytest.mark.parametrize(('playbook_filename', 'messages'), (

pytest.param(PLAY_IMPORT_ROLE,

['only when shell functionality is required'],

id='IMPORT_ROLE',

),

pytest.param(PLAY_IMPORT_ROLE_INLINE,

['only when shell functionality is require'],

id='IMPORT_ROLE_INLINE',

),

pytest.param(PLAY_INCLUDE_ROLE,

['only when shell functionality is require',

'All tasks should be named'],

id='INCLUDE_ROLE',

),

pytest.param(PLAY_INCLUDE_ROLE_INLINE,

['only when shell functionality is require',

'All tasks should be named'],

id='INCLUDE_ROLE_INLINE',

),

))

def test_import_role2(tmp_path, default_rules_collection, playbook_filename, messages):

playbook = _setup_play(tmp_path, playbook_filename)

runner = Runner(default_rules_collection, playbook, [], [], [])

results = runner.run()

for message in messages:

assert message in str(results)

@pytest.mark.parametrize(('playbook_filename', 'error'), (

pytest.param(PLAY_IMPORT_ROLE_INCOMPLETE,

"Failed to find required 'name' key in import_role",

id='IMPORT_ROLE_INCOMPLETE',

),

))

def test_invalid_import_role(tmp_path, default_rules_collection, playbook_filename, error):

playbook = _setup_play(tmp_path, playbook_filename)

runner = Runner(default_rules_collection, playbook, [], [], [])

@pytest.fixture

def playbook_path(request, tmp_path):

playbook_text = request.param

role_tasks_dir = tmp_path / 'test-role' / 'tasks'

role_tasks_dir.mkdir(parents=True)

(role_tasks_dir / 'main.yml').write_text(ROLE_TASKS_MAIN)

(role_tasks_dir / 'world.yml').write_text(ROLE_TASKS_WORLD)

play_path = tmp_path / 'playbook.yml'

play_path.write_text(playbook_text)

return str(play_path)

@pytest.mark.parametrize(('playbook_path', 'messages'), (

pytest.param(PLAY_IMPORT_ROLE,

['only when shell functionality is required'],

id='IMPORT_ROLE',

),

pytest.param(PLAY_IMPORT_ROLE_INLINE,

['only when shell functionality is require'],

id='IMPORT_ROLE_INLINE',

),

pytest.param(PLAY_INCLUDE_ROLE,

['only when shell functionality is require',

'All tasks should be named'],

id='INCLUDE_ROLE',

),

pytest.param(PLAY_INCLUDE_ROLE_INLINE,

['only when shell functionality is require',

'All tasks should be named'],

id='INCLUDE_ROLE_INLINE',

),

), indirect=('playbook_path', ))

def test_import_role2(default_rules_collection, playbook_path, messages):

runner = Runner(default_rules_collection, playbook_path, [], [], [])

results = runner.run()

for message in messages:

assert message in str(results)

@pytest.mark.parametrize(('playbook_path', 'error'), (

pytest.param(PLAY_IMPORT_ROLE_INCOMPLETE,

"Failed to find required 'name' key in import_role",

id='IMPORT_ROLE_INCOMPLETE',

),

), indirect=('playbook_path', ))

def test_invalid_import_role(default_rules_collection, playbook_path, error):

runner = Runner(default_rules_collection, playbook_path, [], [], [])

Didn't know about indirect, neat. Though I am not sure that at first glance, it makes things much easier to follow. It's a lot of pytest "shazam !", but when you know the trick, there is no more magic...

but when you know the trick, there is no more magic...

Right: “Any sufficiently advanced technology is indistinguishable from magic.” --Arthur C. Clarke

I think this is true for anything new to us. I knew about this feature for years before I actually stopped by to read the docs on how do I apply it to my fixtures.

In my opinion, it's important to document such things as a usage guide. Mostly, it'd be useful to have such fixtures in conftest.py, reused everywhere along with some guidelines. It'd confuse newbies, no doubt. And I am often having a hard time finding that golden mean between "too complicated" and "too much copy-paste".

Contributes to #725 Co-Authored-By: Sviatoslav Sydorenko <webknjaz@redhat.com>

webknjaz · 2020-04-23T21:19:48Z

Thanks, @cans!

cans mentioned this pull request Apr 19, 2020

Issue 739/hazardous module loading #741

Closed

webknjaz added status/in-progress labels Apr 20, 2020