[tests] Add permissions tests by tiredPotato · Pull Request #4302 · sosreport/sos

tiredPotato · 2026-04-20T09:13:20Z

Adding tests for preserving file and directory permissions. And renaming symlinks_tests for consistency.

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

Is the commit message split over multiple lines and hard-wrapped at 72 characters?
Is the subject and message clear and concise?
Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
Does the commit contain a Signed-off-by: First Lastname email@example.com?
Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
Are all passwords or private data gathered by this PR obfuscated?

packit-as-a-service · 2026-04-20T09:16:00Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo dnf install -y 'dnf*-command(copr)'
dnf copr enable packit/sosreport-sos-4302
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

pmoravec · 2026-05-14T16:52:01Z

Originally I thought an easier approach would be having the files like /etc/no_perms_file.conf directly in sos/tests, like sos/tests/unittests/path/to/leaf, and dont bother by creating and chmod-ing them in the code.

But since we need to test file permissions and can't rely on umask or manual changes done on the system where avocado is run, the current approach is safer.

Let me review the particular code..

pmoravec · 2026-05-14T17:17:51Z

+    test_file1 = '/etc/no_perms_file.conf'
+    test_file2 = '/var/log/some_perms_file.log'


Worth using more relevant names like noperms_file or someperms_file (or no_perms_file and some_perms_file)?

pmoravec · 2026-05-14T17:19:49Z

+    test_file2 = '/var/log/some_perms_file.log'
+
+    def pre_sos_setup(self):
+        process.run('touch /etc/no_perms_file.conf')


Worth using the variable here, i.e. process.run(f'touch {test_file1}') (depends on variable renaming). If you change the filename, no need to replace the code on multiple places.

This applies to other filenames as well.

pmoravec · 2026-05-14T17:23:30Z

+        extracted_permissions = stat.S_IMODE(
+            os.lstat(extracted_file).st_mode)
+        self.assertEqual(
+            extracted_permissions, 0o000,
+            f"Permissions not preserved: expected 0o000, "
+            f"got {oct(extracted_permissions)}"
+        )
+
+        self.assertFileCollected(self.test_file2)
+        extracted_file = self.get_name_in_archive(self.test_file2)
+        extracted_permissions = stat.S_IMODE(
+            os.lstat(extracted_file).st_mode)
+        self.assertEqual(
+            extracted_permissions, 0o666,
+            f"Permissions not preserved: expected 0o666, "
+            f"got {oct(extracted_permissions)}"
+        )


Rather than copy&pasting the code, isnt it better to have:

for fname, mode in [(self.test_file1, 0o000), (self.test_file2, 0o666)]: assert file collected extract file assert permissions

? Iterating over just 2 tuples is a border case, you make the code shorter and prone to copy&paste error, but add some complexity as well..

pmoravec · 2026-05-14T17:25:36Z

+        os.mkdir(self.test_root)
+        os.mkdir(f'{self.test_root}/d1')
+        os.mkdir(f'{self.test_root}/d1/d2')
+        os.mkdir(f'{self.test_root}/d1/d2/d3')


os.makedirs(f'{self.test_root}/d1/d2/d3', exist_ok=True) is elegant one-liner for the same ;-)

pmoravec · 2026-05-14T17:27:07Z

+
+    def test_top_dir_permissions_preserved(self):
+        extracted_file = self.get_name_in_archive(
+            '/var/tmp/permissions-test/d1')


Worth using test_root as well (ditto 'touch {test_file1}' above)?

Same applies to further filepaths.

jcastill · 2026-06-11T12:56:50Z

@tiredPotato make sure you sync your branch to get the latest commits.
I'm checking the four test errors in cs10:

 (060/132) /__w/sos/sos/sos/tests/report_tests/permissions_tests/permissions_test.py:FilePermissionsPreserved.test_archive_created:  ERROR: stat: path should be string, bytes, os.PathLike or integer, not NoneType (0.02 s)

 (062/132) /__w/sos/sos/sos/tests/report_tests/permissions_tests/permissions_test.py:DirectoryPermissionsPreserved.test_nested_dir_permissions_preserved:  ERROR: expected str, bytes or os.PathLike object, not NoneType (0.04 s)

 (063/132) /__w/sos/sos/sos/tests/report_tests/permissions_tests/permissions_test.py:DirectoryPermissionsPreserved.test_deep_nested_dir_permissions_preserved:  ERROR: expected str, bytes or os.PathLike object, not NoneType (0.04 s)

 (064/132) /__w/sos/sos/sos/tests/report_tests/permissions_tests/permissions_test.py:DirectoryPermissionsPreserved.test_archive_created:  ERROR: stat: path should be string, bytes, os.PathLike or integer, not NoneType (0.02 s)

So far I haven't been able to reproduce locally, but I'm wondering it the failure is a result of a race condition with the tar xf command.

tiredPotato · 2026-06-11T13:44:52Z

@tiredPotato make sure you sync your branch to get the latest commits. I'm checking the four test errors in cs10:

 (060/132) /__w/sos/sos/sos/tests/report_tests/permissions_tests/permissions_test.py:FilePermissionsPreserved.test_archive_created:  ERROR: stat: path should be string, bytes, os.PathLike or integer, not NoneType (0.02 s)

 (062/132) /__w/sos/sos/sos/tests/report_tests/permissions_tests/permissions_test.py:DirectoryPermissionsPreserved.test_nested_dir_permissions_preserved:  ERROR: expected str, bytes or os.PathLike object, not NoneType (0.04 s)

 (063/132) /__w/sos/sos/sos/tests/report_tests/permissions_tests/permissions_test.py:DirectoryPermissionsPreserved.test_deep_nested_dir_permissions_preserved:  ERROR: expected str, bytes or os.PathLike object, not NoneType (0.04 s)

 (064/132) /__w/sos/sos/sos/tests/report_tests/permissions_tests/permissions_test.py:DirectoryPermissionsPreserved.test_archive_created:  ERROR: stat: path should be string, bytes, os.PathLike or integer, not NoneType (0.02 s)

So far I haven't been able to reproduce locally, but I'm wondering it the failure is a result of a race condition with the tar xf command.

@jcastill No change. I couldn't reproduce it either.

Signed-off-by: Adriana Jurkechova <ajurkech@redhat.com>

tiredPotato · 2026-06-12T12:54:10Z

I found this in logs:
stderr: b'tar (child): xz: Cannot exec: No such file or directory\ntar (child): Error is not recoverable: exiting now\ntar: Child returned status 2\ntar: Error is not recoverable: exiting now\n'
so it seems xz is just not installed on the CI runner. I've pushed a workaround which uses the python libraries instead.

pmoravec · 2026-06-12T16:53:46Z

I found this in logs: stderr: b'tar (child): xz: Cannot exec: No such file or directory\ntar (child): Error is not recoverable: exiting now\ntar: Child returned status 2\ntar: Error is not recoverable: exiting now\n' so it seems xz is just not installed on the CI runner. I've pushed a workaround which uses the python libraries instead.

Good catch! Does it make sense to add xz installed by default, e.g. inside the @arif-ali's changes in #4345 ?

(pros: we wont fall into this trap the next time. negs: in 99% cases, xz will be installed redundantly. I am bit inclined to to decline my own idea..)

pmoravec · 2026-06-12T16:56:53Z

+        self.add_copy_spec('/etc/no_perms_file.conf')
+        self.add_copy_spec('/var/log/some_perms_file.log')
+        self.add_copy_spec('/var/tmp/permissions-test')


Very nitpick: You can have one call:

self.add_copy_spec([ '/etc/no_perms_file.conf', '/var/log/some_perms_file.log', '/var/tmp/permissions-test' ])

(if this is the only feedback, let ignore it)

jcastill added the Kind/Testing Related to Testing label Apr 20, 2026

jcastill reviewed Apr 20, 2026

View reviewed changes

Comment thread tests/report_tests/permissions_tests/permissions.py Outdated

tiredPotato force-pushed the permissions_tests branch from 3869bdb to 25e6439 Compare April 20, 2026 11:27

tiredPotato requested a review from pmoravec May 13, 2026 09:14

pmoravec reviewed May 14, 2026

View reviewed changes

tiredPotato force-pushed the permissions_tests branch 2 times, most recently from b8e8bb4 to 93b51e4 Compare June 11, 2026 10:30

tiredPotato force-pushed the permissions_tests branch from 93b51e4 to 638bb0d Compare June 11, 2026 13:03

tiredPotato added 2 commits June 12, 2026 14:14

[tests] Add permissions tests

3f0e2c5

Signed-off-by: Adriana Jurkechova <ajurkech@redhat.com>

[tests] Rename symlinks_tests folder for consistency

165617e

Signed-off-by: Adriana Jurkechova <ajurkech@redhat.com>

tiredPotato force-pushed the permissions_tests branch from 638bb0d to 165617e Compare June 12, 2026 12:15

tiredPotato requested review from jcastill and pmoravec June 12, 2026 12:54

pmoravec reviewed Jun 12, 2026

View reviewed changes

pmoravec approved these changes Jun 12, 2026

View reviewed changes

pmoravec added Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer Status/Needs Review This issue still needs a review from project members labels Jun 12, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tests] Add permissions tests#4302

[tests] Add permissions tests#4302
tiredPotato wants to merge 2 commits into
sosreport:mainfrom
tiredPotato:permissions_tests

tiredPotato commented Apr 20, 2026

Uh oh!

packit-as-a-service Bot commented Apr 20, 2026

Uh oh!

Uh oh!

pmoravec commented May 14, 2026

Uh oh!

pmoravec May 14, 2026

Uh oh!

pmoravec May 14, 2026

Uh oh!

pmoravec May 14, 2026

Uh oh!

pmoravec May 14, 2026

Uh oh!

pmoravec May 14, 2026 •

edited

Loading

Uh oh!

jcastill commented Jun 11, 2026

Uh oh!

tiredPotato commented Jun 11, 2026 •

edited

Loading

Uh oh!

tiredPotato commented Jun 12, 2026

Uh oh!

pmoravec commented Jun 12, 2026

Uh oh!

pmoravec Jun 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		test_file1 = '/etc/no_perms_file.conf'
		test_file2 = '/var/log/some_perms_file.log'

Conversation

tiredPotato commented Apr 20, 2026

Uh oh!

packit-as-a-service Bot commented Apr 20, 2026

Uh oh!

Uh oh!

pmoravec commented May 14, 2026

Uh oh!

pmoravec May 14, 2026

Choose a reason for hiding this comment

Uh oh!

pmoravec May 14, 2026

Choose a reason for hiding this comment

Uh oh!

pmoravec May 14, 2026

Choose a reason for hiding this comment

Uh oh!

pmoravec May 14, 2026

Choose a reason for hiding this comment

Uh oh!

pmoravec May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcastill commented Jun 11, 2026

Uh oh!

tiredPotato commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiredPotato commented Jun 12, 2026

Uh oh!

pmoravec commented Jun 12, 2026

Uh oh!

pmoravec Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pmoravec May 14, 2026 •

edited

Loading

tiredPotato commented Jun 11, 2026 •

edited

Loading