Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

conftest: add docs and tests regarding CiTestCase's subp functionality #343

Merged
merged 3 commits into from
May 12, 2020

Conversation

OddBloke
Copy link
Collaborator

@OddBloke OddBloke commented May 4, 2020

And fix up an inconsistency in tests/helpers.py too by raising a TypeError when subp is called with no arguments as it does under normal operation:

>>> from cloudinit.util import subp
>>> subp()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: subp() missing 1 required positional argument: 'args'

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber stamp +1 😄

I don't understand why we have a separate subp or why we're disabling it's usage in tests, but the changes here make sense.

@OddBloke
Copy link
Collaborator Author

I don't understand why we have a separate subp

util.subp is a wrapper around subprocess.Popen, you can see it at https://github.com/canonical/cloud-init/blob/master/cloudinit/util.py#L2014. Honestly, as it was first written when we still supported Python 2.6 (and perhaps even earlier), there's probably a lot of it that could be dropped in favour of the stdlib at this point, but that would be a fairly large chunk of work.

or why we're disabling it's usage in tests, but the changes here make sense.

We've run into problems where tests pass in particular environments because the code under test is shelling out and getting the expected result back from the executable it ran, but fail in others. This ensures that we don't have any accidental dependency in our tests on binaries outside of cloud-init. (Also, given the things that cloud-init shells out to do to a system, it's in our interests to be very careful about this sort of thing.)

"""Test that disable_subp_usage doesn't impact CiTestCase's subp logic."""

def test_using_subp_raises_assertion_error(self):
with pytest.raises(Exception):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should fix the comment 'assertion_error'. Or fix the _fake_subp to raise an assertion_error . Otherwise this is confusing. Your test is named ....raises_assertion_error... and then it insists that the more generic Exception is raised.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this was a copy/paste error from the above tests, will fix.

(The pytest functionality raises AssertionError but I didn't want to change the behaviour of CiTestCase "just" for the sake of consistency.)

cloudinit/tests/test_conftest.py Outdated Show resolved Hide resolved
OddBloke added 3 commits May 12, 2020 12:29
This more accurately mirrors normal behaviour:

```py
>>> from cloudinit.util import subp
>>> subp()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: subp() missing 1 required positional argument: 'args'
```
@OddBloke
Copy link
Collaborator Author

Thanks for the review, Scott! I believe I've addressed both your comments.

@OddBloke OddBloke requested a review from smoser May 12, 2020 16:29
@OddBloke OddBloke merged commit c8f20b3 into canonical:master May 12, 2020
@OddBloke OddBloke deleted the tests branch May 12, 2020 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants