MAINT: Do not use deprecated mktemp()

[lyzlisa]

Closes #12071

• Changed use of mktemp to mkstemp since mktemp was deprecated
• No change required for numpy/lib/tests/test_format.py
• Coverage report: 84%

[rossbar]

One difference between mktemp and mkstemp is that the latter creates the file handle, which the caller is then responsible for closing. I'm not sure how the files are handled in the modified tests, but IMO it's worth double-checking that things are explicitly closed.

[lyzlisa]

In the original code when mktemp() was used, fp was deleted at the end of the test.

With the use of mkstemp(), fp is still being deleted at the end of the test.

Would it be safer to use a context manager to explicitly close the file before deleting it?

[mattip]

These days I think a pytest tmp_path would be cleaner. The test signature would become test_something(self, tmp_path) and pytest magically fills that in with a temporary pathlib object.

[alize-papp]

We used pytest tmp_path in test_memmap.py as suggested.
In test_multiarray.py it feels like we don't need to, since there is:

    def teardown(self):
    shutil.rmtree(self.tempdir)

that already deletes everything. Does that make sense?

[rgommers]

that already deletes everything. Does that make sense?

That sounds right. It's not impossible that a ResourceWarning will still be emitted because of discarding the open file handle, in that case CI will fail.

[mattip]

I would suggest a deeper refactoring, using pytest's tmp_file with no extra fixtures needed in the class setup or teardown. Doesn't this work:

Suggested change

	def test_empty_files_text(self, tmp_filename):
	with open(self.filename, 'w') as f:
	pass
	y = np.fromfile(self.filename)
	assert_(y.size == 0, "Array not empty")
	def test_empty_files_text(self, tmp_file):
	with open(tmp_file, 'w') as f:
	pass
	y = np.fromfile(tmp_file)
	assert_(y.size == 0, "Array not empty")

[mboutet]

@mattip, unless I'm mistaken, tmp_file is not a built-in pytest fixture. You can try by running this dummy test:

def test_foobar(tmp_file):
    pass

It will fail because the fixture does not exist.

Besides, if such built-in fixture existed, it would probably also create the file on disk which is something we don't want for the tests. We only want a filename without pre-creating the file.

[mattip]

Ahh, my bad. It is tmp_path not tmp_file which gives you a PathLib object pointing to a directory like /tmp/pytest-of-matti/pytest-8/test_foobar0 where each invocation of pytest will increment the pytest-8 part and delete old ones according to some logic, and test_foobar0 is the name of the test with a 0 for good measure. So this test would be

def test_empty_files_text(self, tmp_path):
        with open(tmp_path / 'empty', 'w') as f:
            pass
        y = np.fromfile(tmp_path / 'empty')
        assert_(y.size == 0, "Array not empty")

[mboutet]

Ok, but if tmp_path is used directly, then all references to self.filename must be removed. Another benefit of the custom fixture tmp_filename is that it is parametrized to cover two cases with very low effort (filename is a str or filename is a path-like object). Doing so allows to combine at least two tests into one test.

[mattip]

all references to self.filename must be removed

Sounds like a win to me, I am not a big fan of attributes that are overridden for each test. self.something should be a persistent attribute. If it is something temporary for each test then it should be an argument of the test function.

[mattip]

I think setup/teardown is a better paradigm here than using a yield like this. Why do you want to re-create dtype and x?

[lyzlisa]

I think setup/teardown is a better paradigm here than using a yield like this.

It was my understanding that setup/teardown is an anti-pattern when using pytest. Not that they shouldn't be used, but using fixtures is better suited. The documentation details the benefits of using fixtures. There’s also a note at the top of this page on xunitsetup encouraging to use fixtures.

Why do you want to re-create dtype and x?

If a test modifies dtype and/or x, then it will affect all the other tests which violates the principle that tests should be independent and without side effects on the other tests. By ensuring that each test gets a properly initialized x and dtype, we prevent such situation. However, we could remove the delattr calls (and drop the yield) because x and dtype will be overwritten in the next test anyway.

[mattip]

In this case then I think it would be clearer if x was passed in to the tests that need it as an additional argument, rather than stuffing it onto self. Tests that need dtype should just call x.dtype. As I commented elsewhere, this use/abuse of self.x seems weird.

[lyzlisa]

I refactored TestIO to use a fixture (x) and also updated all the self.filename references to use the value from the tmp_filename fixture instead, which remove all attributes stored in self.

Is it too much out of scope to refactor TestMemmap to use fixtures too since the original issue was to remove the deprecated mktemp() function?

[mattip]

I think the TestMemmap is fine as is. I would like to keep the use of fixtures to a minimum, it adds a layer of indirection that is harder to parse.

[mattip]

It is not clear form the code how comma_decimal_point_locale is ever used. A comment would be helpful.

[mattip]

just to be clear: the previous code was also opaque as to what this does

[mattip]

Thanks, I think this is a big improvement.

[mattip]

LGTM, besides one nit for a comment. Would anyone else like to take a look?

[rossbar]

LGTM as well - using pytest features for the tmp file handling is a nice improvement IMO. Thanks @lyzlisa !