Skip to content

Cleanup test_net.py#4840

Merged
TheRealFalcon merged 15 commits into
canonical:mainfrom
TheRealFalcon:test-net
Mar 5, 2024
Merged

Cleanup test_net.py#4840
TheRealFalcon merged 15 commits into
canonical:mainfrom
TheRealFalcon:test-net

Conversation

@TheRealFalcon
Copy link
Copy Markdown
Contributor

Proposed Commit Message

test: cleanup test_net.py

* Move ~3500 lines of network config into its own file
* Replace unittest idioms with pytest idioms
* Remove CiTestCase and FilesystemMockingTestCase
* Parametrize the obvious tests
* Remove commented tests and debug print statements

Additional Context

There's still more that could be done here, but I want to stop here after most of the "easy" changes.

Commits are technically independently applicable, but I think I'd rather squash merge the whole thing as my current commits are fairly noisy. A few of the commits will have some extra context in the commit message though.

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>)

@holmanb holmanb self-assigned this Feb 5, 2024
@github-actions
Copy link
Copy Markdown

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Feb 20, 2024
@holmanb holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Feb 20, 2024
Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Much cleaner to look at and reason about. Thx for catching and documenting the failed/misspelled test-cases.

return_value="root=/dev/sda1",
)
self.tmp_dir = lambda: str(tmpdir)
self.tmp_dir = lambda: str(tmpdir_factory.mktemp("a", numbered=True))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice idea here. good point of reference for comparison of failed test output after the fact too.

self._assert_headers(found)

def test_small_config_v2(self):
entry = NETWORK_CONFIGS["small_v1"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Finally getting test coverage on small_v2 for SUSE. :)

@blackboxsw
Copy link
Copy Markdown
Collaborator

+1 on the squash here. it was helpful for the review to have separate commts, but it's not really a functional set of differences that I'd see future-me wanting to distinguish.

@TheRealFalcon TheRealFalcon merged commit a576d11 into canonical:main Mar 5, 2024
@TheRealFalcon TheRealFalcon deleted the test-net branch March 5, 2024 17:36
catmsred pushed a commit to catmsred/cloud-init that referenced this pull request Mar 11, 2024
test: cleanup test_net.py

* Move ~3500 lines of network config into its own file
* Replace unittest idioms with pytest idioms
* Remove CiTestCase and FilesystemMockingTestCase
* Parametrize the obvious tests
* Remove commented tests and debug print statements

Much of the heavy lifting was done using `pytestify`.
Moving configuration into another file was mainly because the file
is way too large.

We have commented tests that are years old. Git will keep the
history if we ever need to bring them back. I left the TODO
in place as that can still be a reminder that they are needed.
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