tests: pytestified test_cc_rsyslog.py #6622
Conversation
|
@blackboxsw could you please verify this? |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
@MoeSalah1999 please follow https://docs.cloud-init.io/en/latest/development/index.html which documents running tox-e format .
Also:
I don't think basecfg or bsdcfg need to be fixtures, it's also not good practice to share mutable fixtures when the individual tests modify that mutable structure as it'll lead to intermittent test failures when running tests in parallel. Since they are just plain dicts that do not require resource setup, how about just define them as class-level attributes which are manipulated via copy.deepcopy before updating.
|
|
||
|
|
||
| @pytest.fixture | ||
| def temp_dir(): |
There was a problem hiding this comment.
pytest already has a tmpdir fixture, not need to define our own.
There was a problem hiding this comment.
Thanks for clearing that up and for the guidance! I made all the changes you requested, please verify them at your convenience.
|
Additionally please format the proposed commit message suggestion to correctly represent conventional commits. And the footer of a commit message should not say |
| (os.path.join(temp_dir, "default.cfg"), "*.* foohost\n"), | ||
| (os.path.join(temp_dir, "my.cfg"), "abc\n"), | ||
| (os.path.join(temp_dir, "mydir/mycfg"), "filefoo-content\n"), | ||
| (os.path.join(tmpdir, "default.cfg"), "*.* foohost\n"), |
There was a problem hiding this comment.
Sorry for this nit, tmpdir already provides a tmpdir.join method, so we don't need the extra os.path.join calls throughout.
There was a problem hiding this comment.
Honestly I was so eager to make that change, but I didn't wanna mess with anything other than what you instructed me to do. I'll get right on it.
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Please use tmpdir.join where possible otherwise looks good to me. Thank you!
MoeSalah1999
force-pushed
the
pytestify-test_cc_rsyslog.py
branch
from
1e396a6 to
29282d5
Compare
| cfg = copy.deepcopy(self.BSDCFG) | ||
|
|
There was a problem hiding this comment.
self.BSDCFG doesn't appear to be manipulated in this test. So you can just use it directly on line 98, no need to deepcopy.
There was a problem hiding this comment.
Just a suggestion, how about I construct a fresh dict literal in this test only, to avoid silently re-introducing shared mutable states, and to not lose protection against implementation changes in load_config().
| ): | ||
| cloud = get_cloud(distro="freebsd", metadata={}) | ||
| self.assertEqual(load_config({}, distro=cloud.distro), self.bsdcfg) | ||
| assert load_config({}, distro=cloud.distro) == cfg |
There was a problem hiding this comment.
| assert load_config({}, distro=cloud.distro) == cfg | |
| assert load_config({}, distro=cloud.distro) == self.BSDCFG |
| fname = os.path.join(self.tmp, "foo.cfg") | ||
| self.assertEqual([fname], changed) | ||
| self.assertEqual(util.load_text_file(fname), cfgline + "\n") | ||
| fname = tmpdir.join("foo.cfg") |
There was a problem hiding this comment.
let's just str(....) up here insetad of at the two callsites using fname below
| fname = tmpdir.join("foo.cfg") | |
| fname = str(tmpdir.join("foo.cfg")) |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Thank you @MoeSalah1999 again. two minor nits and we'll land this.
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Ship it! Thanks again @MoeSalah1999
-made sure test case classes don't inherit from TestCase -used pytest fixtures instead of unittest setUp method -converted all self.assert to plain assert. Related GH-6427
2 participants
Proposed Commit Message
Additional Context
Test Steps
Merge type