test: pytestified test_cc_growpart.py

[MoeSalah1999]

Proposed Commit Message

Converted test_cc_growpart.py from unittest to pytest:
-Removed all unittest and TestCase imports and inheritances.
-Used pytest fixtures.
-Converted all self.assert to plain assert.
-Made a couple of minor changes to avoid unusual behavior.

Related GH-6427 

Additional Context

Test Steps

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[blackboxsw]

Let's please avoid propagating the use of setUp as a fixture if we can.

• minimally let's make sure to avoid os.scandir("/tmp") to write files directly to the top-level of a host's /tmp directory. We should be using pytest's tmpdir or tmp_path instead.
• I think we can avoid setUp entirely in TestDisabled case
• Please review the approach taken in TestResizeZFS for distro setup which may be more useful that using that plain mock.Mock() seen in the modified setUp.

[blackboxsw]

We may be able to use pytest's mocker fixutre here instead of relying on mock.patch.object.

[blackboxsw]

I don't see the point of having a fixture at all here. This is dict is only used in one place. Let's just inline this response as part of the mocked resizer_factory.

[blackboxsw]

We really should use pytest's tmpdir fixture if we can here to avoid using test constructs which write files directly to the top-level of the host system's /tmp directory. In fact I'm not sure we need any of these operations in setUp anymore. They are antiquated and come from the python unittest framework and a misunderstanding of loggingsetup as well as temp file writing.

[blackboxsw]

Something strange has occurred in this branch. I'm seeing 10 unrelated commits in this PR currently. Can you please rebase against tip of main with something like the following to resolve this patch delta and make sure it only includes your test_cc_growpart.py changes?

git fetch upstream
git rebase -i upstream/main
git push <your_remote_name> pytestify-test-growpath --force

[MoeSalah1999]

Rebased interactively onto upstream/main and cleaned history. The PR now contains only the test_cc_growpart.py changes.

[MoeSalah1999]

@blackboxsw Could you please verify this so it doesn't get closed automatically by github?

[blackboxsw]

Thank you for the updates @MoeSalah1999. It's looking better. Let's get one more pass on this PR please where possible.

[blackboxsw]

These are invalid asserts, they should be of the format:
assert cc_growpart.RESIZE.NOCHANGE == find("/dev/XXda1", resized)[1]

[blackboxsw]

Good fix/correction here, and you are correct for those reasons you mention. I don't think we need to persist this comment in the test though.

Suggested change

	# Instantiated MockDistro, otherwise we would be assigning
	# the class not an instance when we pass self.distro
	# into resize_devices, which expects a distro instance

[blackboxsw]

In shifting to pytest it would be better to use pytest's mocker instead of mock.patch.object in tests you are touching in this PR.

I realize there are a number of pre-existing mock.patch.objects sprinkled throughout this file, so I'm not recommending that you change this in every existing test if you don't feel inclined. But, for tests you do touch, lets please use pytest's mocker.patch.object() instead of mock.patch.object

there is no need to change all existing uses of mock.patch.object in other tests

[blackboxsw]

Same here. Please use the mocker fixture and mocker.patch.object instead.

[blackboxsw]

There is nothing really needed in this setUp method anymore.

Nothing uses self.log, and in pytest we can use caplog or capsys fixtures to look through logs or stdout/error messages.

Let's just remove setUp altogether and create a local distro variable which is used within test_simple_devices.

Other things that look fairly painful in this test (not your fault) are that the test doesn't use mocker for os.stat and instead encloses the whole test a in a try/finally clause to attempt to reset original state of os.stat and the MockDistro class after the unittest completes. This is really bad practice that was in the original test. If possible, I'd like to see if we can avoid that try/finally stuff, drop the setUp fixture and just use mocker instead.