feat(test): Add tests for MOTD

[m-horky]

• Card ID: CCT-414
• Card ID: CCT-415

[m-horky]

Rawhide test failure is addressed by #245

[Glutexo]

Suggested change

	# It stays absent when run multiple times.
	insights_client.update_motd_message()
	assert not (mock_fs.chroot / "etc/motd.d/insights-client").exists()

[Glutexo]

Suggested change

	def test_ignored_on_dev_null(mock_fs):
	# When the /etc/motd.d/insights-client is a symbolic link to /dev/null...
	(mock_fs.chroot / "etc/motd.d/insights-client").symlink_to(pathlib.Path(os.devnull))
	# ...it should not be overwritten...
	insights_client.update_motd_message()
	assert (mock_fs.chroot / "etc/motd.d/insights-client").samefile(os.devnull)
	# ...whether the MOTD file should be present or not.
	(mock_fs.chroot / "etc/insights-client/.registered").open("w").close()
	insights_client.update_motd_message()
	assert (mock_fs.chroot / "etc/motd.d/insights-client").samefile(os.devnull)
	def test_ignored_on_dev_null(mock_fs):
	# When the /etc/motd.d/insights-client is a symbolic link to /dev/null...
	(mock_fs.chroot / "etc/motd.d/insights-client").symlink_to(pathlib.Path(os.devnull))
	# ...it should not be overwritten...
	insights_client.update_motd_message()
	assert (mock_fs.chroot / "etc/motd.d/insights-client").samefile(os.devnull)
	@pytest.mark.parametrize(
	["dot_file"],
	[
	("etc/insights-client/.registered",),
	("etc/insights-client/.unregistered",),
	]
	)
	def test_ignored_on_dev_null_and_dot_file(mock_fs, dot_file):
	# When the /etc/motd.d/insights-client is a symbolic link to /dev/null...
	(mock_fs.chroot / "etc/motd.d/insights-client").symlink_to(pathlib.Path(os.devnull))
	# ...it should not be overwritten whether the MOTD file should be present or not.
	(mock_fs.chroot / dot_file).open("w").close()
	insights_client.update_motd_message()
	assert (mock_fs.chroot / "etc/motd.d/insights-client").samefile(os.devnull)

[m-horky]

Yeah. I still don't agree. These tests are state-based, not action-based. The unit test ensures the files is (not) present, and we verify it through one or two paths to get there. Let's not go philosophical.

[Glutexo]

The philosophy behind the code is as important as the code itself. Otherwise it’s not possible to be correct and consistent.

[Glutexo]

I thought up a way how to get the paths from the patches in a more cocsise may. I am not entirely sure about the semantics, but it doesn’t feel completely off either.

Suggested change

	def __getitem__(self, key):
	self.patches[key].new

A magic method like this would allow calls like mock_fs["insights_client.MOTD_FILE"]. Imagine:

assert not mock_fs["insights_client.MOTD_FILE"].exists()

[m-horky]

It's getting complicated. It is not a production code. I prefer readability over smartness in tests, it can save you significant amount of time when it goes wrong.

[ptoscano]

assert not mock_fs["insights_client.MOTD_FILE"].exists()

IMHO this is not logically correct: the idea of the tests is to check for the actual paths, not on the names of the mocks (which are the internal modules/variables in the code).

[Glutexo]

Here, though, the internal name only serves as a string key to grab the path defined here in the tests.

[Glutexo]

The whole MockFS class with its seven methods already brings a great amount of smartness, and thus reducing streamlineliness.

[ptoscano]

Here, though, the internal name only serves as a string key to grab the path defined here in the tests.

That's still missing the point of what I wrote above: the fact that there are mocks to redirect the file operation is a semi-internal implementation detail of the test. The point of the test is checking the resulting paths (that they exist, or what they are, etc), and thus it is correct to use their names in checks/assertions. Once the mocks are set up, from the POV of the tests they should "not be seen" (other than using mock_fs for checks).

So no to your proposal: using the names of the mocks in tests is logically wrong.

[ptoscano]

The whole MockFS class with its seven methods already brings a great amount of smartness, and thus reducing streamlineliness.

What is "streamlineliness" supposed to mean exactly? And mostly important, what's the whole point of your sentence? Please: rather than using nicely sounding words/sentences, provide actual/actionable points.

[m-horky]

I've introduced further changes that should make the tests more readable. The MockFS now wraps the calls we're using (touch, exists) in a small interface that abstracts away the chroot completely.

[ptoscano]

assert not mock_fs["insights_client.MOTD_FILE"].exists()

IMHO this is not logically correct: the idea of the tests is to check for the actual paths, not on the names of the mocks (which are the internal modules/variables in the code).

[Archana-PandeyM]

motd tests failed downstream. Please analyse the failure.

[Archana-PandeyM]

LGTM. Downstream tests failed because those are running on older version of insights.

[Glutexo]

Although the touch command is often (mis)used to create a file, it’s primary intended purpose is to set a timestamp. Since this method only mimics the side-effect and not the original function, the name is misleading.

[m-horky]

from touch (5):

Update the access and modification times of each FILE to the current time.
A FILE argument that does not exist is created empty,

The first paragraph documents this behavior. It is not misleading, touch is really common when you want to create an empty file on UNIX-like systems.