Add more integration tests #615

lucasmoura · 2020-10-16T15:10:57Z

Translate tests from cloud_test to the new integration test framework.

We are translating the following tests:

test_runcmd.py
seed_random_data.py
set_hostname.py
set_hostname_fqdn.py
snap.py
ssh_auth_key_fingerprints_disable.py
ssh_auth_key_fingerprints_enable.py
ssh_import_id.py
ssh_keys_generate.py
ssh_keys_provided.py
timezone.py
write_files.py

OddBloke

Thanks for this change Lucas! You've clearly done a great job of understanding both the old and new test frameworks. My comments inline are generally either requests for docstring cleanup/clarification, or suggest that we parameterise some tests which I think would lend themselves to it well; this latter suggestion will involve understanding how class_client works (vs client), and you can see an example of this in test_users_groups.py.

OddBloke · 2020-10-16T17:16:38Z

tests/integration_tests/modules/test_runcmd.py

+"""Integration test for the runcmd module.
+This test specifies a command to be executed by the ``runcmd`` module


Per PEP-257, which we generally try to follow:

Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description.

Suggested change

"""Integration test for the runcmd module.

This test specifies a command to be executed by the ``runcmd`` module

"""Integration test for the runcmd module.

This test specifies a command to be executed by the ``runcmd`` module

(This is a nit, but an easily fixable one.)

tests/integration_tests/modules/test_set_hostname.py

OddBloke · 2020-10-16T17:39:29Z

tests/integration_tests/modules/test_set_hostname.py

@@ -0,0 +1,43 @@
+"""Integration test for the set_hostname module.
+This test specifies a hostname and fqdn through the ``set_hostname`` module


This test

This file is two tests, we should describe each of them separately. (This is particularly important because they are two test methods in a single class: people may very easily assume that you're using a class_client based on this description, which makes it harder for them to understand what is going on.)

OddBloke · 2020-10-16T17:40:41Z

tests/integration_tests/modules/test_snap.py

@@ -0,0 +1,27 @@
+"""Integration test for the snap module.
+This test specifies a command to be executed by the ``runcmd`` module


runcmd

This needs an update.

OddBloke · 2020-10-16T17:43:16Z

tests/integration_tests/modules/test_snap.py

+        assert "core" in snap_output
+        assert "hello-world" in snap_output


Perhaps worth adding spaces here to reduce the likelihood that we match substrings elsewhere in the output?

Suggested change

assert "core" in snap_output

assert "hello-world" in snap_output

assert "core " in snap_output

assert "hello-world " in snap_output

OddBloke · 2020-10-16T18:25:56Z

tests/integration_tests/modules/test_ssh_keys_provided.py

+
+
+USER_DATA = """\
+  #cloud-config


This is all indented by two more spaces than necessary.

OddBloke · 2020-10-16T18:26:22Z

tests/integration_tests/modules/test_ssh_keys_provided.py

+class TestSshKeysProvided:
+
+    @pytest.mark.user_data(USER_DATA)
+    def test_ssh_keys_provided(self, client):


This feels like another good candidate for parameterisation, what do you think?

OddBloke · 2020-10-16T18:37:37Z

tests/integration_tests/modules/test_write_files.py

+# NOTE: on trusty 'file' has an output formatting error for binary files and
+#       has 2 spaces in 'LSB  executable', which causes a failure here
+#


We don't expect to run these tests against trusty, so I think we can drop this disclaimer:

Suggested change

# NOTE: on trusty 'file' has an output formatting error for binary files and

# has 2 spaces in 'LSB executable', which causes a failure here

#

OddBloke · 2020-10-16T18:38:40Z

tests/integration_tests/modules/test_write_files.py

+class TestWriteFiles:
+
+    @pytest.mark.user_data(USER_DATA)
+    def test_write_files(self, client):


Another good candidate for parameterisation?

OddBloke · 2020-10-16T19:00:12Z

tests/integration_tests/modules/test_write_files.py

+#cloud-config
+write_files:
+-   encoding: b64
+    content: CiMgVGhpcyBmaWxlIGNvbnRyb2xzIHRoZSBzdGF0ZSBvZiBTRUxpbnV4


As we are defining this string in Python, what do you think to doing something like:

ASCII_TEXT = "ASCII text" B64_CONTENT = base64.b64encode(ASCII_TEXT)

and then using .format() to include it in USER_DATA?

This saves us from having the "ASCII text" string defined only in the assertion code, which I think makes it easier to parse what is going on.

lucasmoura · 2020-10-16T21:27:23Z

Thanks for the review @OddBloke. I think I have addressed all of the comments

OddBloke

Thanks Lucas, this is really good work! I have a couple of minor doc nits, which I'll apply myself before landing.

(I also have started a convo with James inline, perhaps we should take that to a different forum.)

tests/integration_tests/modules/test_ssh_auth_key_fingerprints.py

OddBloke · 2020-10-19T18:11:40Z

tests/integration_tests/modules/test_package_update_upgrade_install.py

+@pytest.mark.user_data(USER_DATA)
+class TestPackageUpdateUpgradeInstall:
+
+    def assert_package_installed(self, pkg_out, name, version=None):


@TheRealFalcon This assertion helper mirrors one in the cloud_tests/ which is used in multiple places: do you have any thoughts as to whether we should store such shared "test" code on client, or elsewhere?

(No change needed as part of this PR, to be clear!)

Yeah, that's a good question. I'm worried about seeing every conceivable helper function going into that class with implementations being nothing more than a series of execs or push/pulls and parsing what's returned, though I realize we're kind of already doing that with the install_* and read/write methods.

Would it make sense to start putting these kinds of helper functions into separate files (not named util.py) and have the helper function take an IntegrationClient as an argument?

tests/integration_tests/modules/test_package_update_upgrade_install.py

OddBloke · 2020-10-19T21:28:14Z

On Mon, Oct 19, 2020 at 01:41:47PM -0700, James Falcon wrote: Yeah, that's a good question. I'm worried about seeing every conceivable helper function going into that class with implementations being nothing more than a series of execs or push/pulls and parsing what's returned, though I realize we're kind of already doing that with the `install_*` and read/write methods.

Yeah, agreed, I think it will make that namespace extremely noisy.

Would it make sense to start putting these kinds of helper functions into separate files (not named util.py) and have the helper function take an `IntegrationClient` as an argument?

In cloud-test-framework we had a class called `InstanceInteractions`, which defined a bunch of these sorts of things (a quick sample: `get_file`, `put_file`, `apt_get_update`) and in the instance class's `__init__` we had: `self.interactions = InstanceInteractions(self)`. That gives you a separate namespace (`instance.interactions.`) which is accessible directly on the objects to which it applies, and which handles passing in the correct first argument for you. I think this would work pretty well here too, what do you think? (One note: asserting was handled completely differently in c-t-f, so we may find that having several namespaces might suit our purposes better: .interactions and .assertions, for example).

TheRealFalcon · 2020-10-19T22:00:33Z

Yeah, that's a good option too. It makes the calling semantics a nicer. I do get a little nervous about having the IntegrationClient "own" these additional modules when it doesn't really need to. Tradeoffs either way I guess.

lucasmoura force-pushed the add-more-integration-tests branch from b10805f to 46fc4ac Compare October 16, 2020 15:11

OddBloke requested changes Oct 16, 2020

View reviewed changes

lucasmoura force-pushed the add-more-integration-tests branch from 46fc4ac to 7214b39 Compare October 16, 2020 21:26

lucasmoura requested a review from OddBloke October 16, 2020 21:27

Add more integration tests

1b4d1e9

lucasmoura force-pushed the add-more-integration-tests branch from 7214b39 to 1b4d1e9 Compare October 19, 2020 15:13

OddBloke approved these changes Oct 19, 2020

View reviewed changes

OddBloke and others added 2 commits October 19, 2020 14:39

Apply suggestions from code review

309c3aa

Oops.

dafb9f2

OddBloke merged commit b94962b into canonical:master Oct 19, 2020

This was referenced May 12, 2023

cloud-init regenerating ssh-keys #3753

Closed

Release 20.4 #3811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more integration tests #615

Add more integration tests #615

lucasmoura commented Oct 16, 2020

OddBloke left a comment

OddBloke Oct 16, 2020

OddBloke Oct 16, 2020

OddBloke Oct 16, 2020

OddBloke Oct 16, 2020

OddBloke Oct 16, 2020

OddBloke Oct 16, 2020

OddBloke Oct 16, 2020

OddBloke Oct 16, 2020

OddBloke Oct 16, 2020

lucasmoura commented Oct 16, 2020

OddBloke left a comment

OddBloke Oct 19, 2020

TheRealFalcon Oct 19, 2020

OddBloke commented Oct 19, 2020 via email

TheRealFalcon commented Oct 19, 2020

		"""Integration test for the runcmd module.
		This test specifies a command to be executed by the ``runcmd`` module

		@@ -0,0 +1,43 @@
		"""Integration test for the set_hostname module.
		This test specifies a hostname and fqdn through the ``set_hostname`` module

		@@ -0,0 +1,27 @@
		"""Integration test for the snap module.
		This test specifies a command to be executed by the ``runcmd`` module

		assert "core" in snap_output
		assert "hello-world" in snap_output

	# NOTE: on trusty 'file' has an output formatting error for binary files and
	# has 2 spaces in 'LSB executable', which causes a failure here
	#

Add more integration tests #615

Add more integration tests #615

Conversation

lucasmoura commented Oct 16, 2020

OddBloke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmoura commented Oct 16, 2020

OddBloke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OddBloke commented Oct 19, 2020 via email

TheRealFalcon commented Oct 19, 2020