PR#4326: Remove fp file using os.unlink instead of using the delete option of NamedTemporaryFile - koji

koji

#4326 Remove fp file using os.unlink instead of using the delete option of NamedTemporaryFile

Merged 3 months ago by tkopecek. Opened 3 months ago by alisboav.

alisboav/koji issue-4320 into master

Remove fp file using os.unlink instead of using delete option of NamedTemporaryFile

root • 3 months ago

5e3b697

file modified

+7 -3

		`@@ -1,5 +1,6 @@`
		`from unittest import mock`
		`import tempfile`
		`+ import os`
		`import unittest`

		`import koji`
		`@@ -38,22 +39,24 @@`
		`self.task.get_nvrp('/dev/null/non_existent_path')`

		`# empty file`
		`- with tempfile.NamedTemporaryFile(delete_on_close=False) as fp:`
		`+ with tempfile.NamedTemporaryFile(delete=False) as fp:`
		`fp.write(b'')`
		`fp.close()`
		`with self.assertRaises(koji.GenericError):`
		`self.task.get_nvrp(fp.name)`
		`+ os.unlink(fp.name)`

		`# empty xml`
		`- with tempfile.NamedTemporaryFile(delete_on_close=False) as fp:`
		`+ with tempfile.NamedTemporaryFile(delete=False) as fp:`
		`fp.write(b'<?xml version="1.0"?><test></test>')`
		`fp.close()`
		`with self.assertRaises(koji.GenericError):`
		`self.task.get_nvrp(fp.name)`
		`+ os.unlink(fp.name)`

		`def test_get_nrvp_correct(self):`
		`# minimal correct xml`
		`- with tempfile.NamedTemporaryFile(delete_on_close=False) as fp:`
		`+ with tempfile.NamedTemporaryFile(delete=False) as fp:`
		`fp.write(b'''<?xml version="1.0" encoding="utf-8"?>`
		`<image schemaversion="7.4" name="Fedora-34.0_disk">`
		`<profiles>`
		`@@ -70,6 +73,7 @@`
		`self.assertEqual(name, 'Fedora-34.0_disk')`
		`self.assertEqual(version, '1.0.0')`
		`self.assertEqual(profile, 'Base')`
		`+ os.unlink(fp.name)`

		`def test_handler_correct(self, arches=None):`
		`if arches is None:`

alisboav commented 3 months ago

Using python3.9 (default python of AlmaLinux9) there are two tests failing:

=========================== short test summary info ============================
FAILED tests/test_plugins/test_kiwi_builder.py::TestKiwiBuildTask::test_get_nrvp_correct - TypeError: NamedTemporaryFile() got an unexpected keyword argument 'delete_...
FAILED tests/test_plugins/test_kiwi_builder.py::TestKiwiBuildTask::test_get_nrvp_invalid_xml - TypeError: NamedTemporaryFile() got an unexpected keyword argument 'delete_...
================== 2 failed, 2694 passed in 63.92s (0:01:03) ===================

There is no delete_on_close option using python3.9 - https://docs.python.org/3.9/library/tempfile.html

In python3.12:
> If delete is true (the default) and delete_on_close is false, the file is deleted on context manager exit only, or else when the file-like object is finalized.

The aims of this PR is to use os.unlink instead of the delete option of NamedTemporaryFile and make it work regardless of python version:

======================= 2696 passed in 61.69s (0:01:01) ========================

Fixes https://pagure.io/koji/issue/4320

mikem commented 3 months ago

Seems fine to me. This unit test is the only place we have used the option.

Actually this unit test goes against the normal pattern we have in most of our unit tests, which is to set up a temporary dir in setUp and remove it in tearDown. I think this fix is fine for now, but would be nice to bring this in line with the rest.