should the functional-testing scratch_file() utility guarantee that the path it returns doesn't exist?

should the functional-testing scratch_file() utility guarantee that the path it returns doesn't exist?

[Peter Maydell]

The functional testing framework includes a utility function
scratch_file() which you can use to get a path for a scratch
file to use in the test. However, it doesn't do anything to
ensure that the path it returns doesn't already exist.
Should it?

I ran into this with the aarch64/test_rme_sbsaref.py test:
this does:
        rme_stack = self.scratch_file('.')
to get a scratch path, and then uses it both to place
specific files:
        pflash0 = join(rme_stack, 'out', 'SBSA_FLASH0.fd')
and to place subdirectories:
        efi = join(rme_stack, 'out', 'EFI')
        os.makedirs(efi, exist_ok=True)

In the original version of this test we used os.mkdir(efi),
but this fails sometimes because the directory already exists
(typically if the test was interrupted during a previous run);
see this email for the error log:
https://lore.kernel.org/qemu-devel/CAFEAcA_ZQ13qMRUQsieJiEPV=ULrDbz8=EJaW4_kw=yEysob0w@mail.gmail.com/

But this seems a bit of a hacky workaround to me -- shouldn't the
test framework guarantee to the test that it's returning a
path that doesn't already have something there?

Secondary question: is it OK to pass '.' to this function
and then construct filenames based on the return value,
or would it be better to call scratch_file('out', 'SBSA_FLASH0.fd')
etc and have the utility function construct the whole path?

thanks
-- PMM

Re: should the functional-testing scratch_file() utility guarantee that the path it returns doesn't exist?

[Daniel P. Berrangé]

On Mon, Sep 08, 2025 at 02:13:54PM +0100, Peter Maydell wrote:
> The functional testing framework includes a utility function
> scratch_file() which you can use to get a path for a scratch
> file to use in the test. However, it doesn't do anything to
> ensure that the path it returns doesn't already exist.
> Should it?

No, we need to be able to use scratch_file() to refer to
previously created resources. For example, when we extract
files from an archive, we want to then reference those, or
when we uncompress a file.

> I ran into this with the aarch64/test_rme_sbsaref.py test:
> this does:
>         rme_stack = self.scratch_file('.')
> to get a scratch path, and then uses it both to place
> specific files:
>         pflash0 = join(rme_stack, 'out', 'SBSA_FLASH0.fd')
> and to place subdirectories:
>         efi = join(rme_stack, 'out', 'EFI')
>         os.makedirs(efi, exist_ok=True)
> 
> In the original version of this test we used os.mkdir(efi),
> but this fails sometimes because the directory already exists
> (typically if the test was interrupted during a previous run);
> see this email for the error log:
> https://lore.kernel.org/qemu-devel/CAFEAcA_ZQ13qMRUQsieJiEPV=ULrDbz8=EJaW4_kw=yEysob0w@mail.gmail.co/m

We have a tearDown function which purges everything under the
scratch dir location, so if a test is interrupted in a "normal"
way we should be safe on re-run. Only if python is hard-terminated
would the tearDown method fail to get run.

Perhaps we should proactively purge the scratch location in
the setUp method too, as a safety net against previous unclean
shutdown, as the intent of the 'scratch file' concept is that
the test should have a clean environment when it starts running.


> Secondary question: is it OK to pass '.' to this function
> and then construct filenames based on the return value,
> or would it be better to call scratch_file('out', 'SBSA_FLASH0.fd')
> etc and have the utility function construct the whole path?

My recommendation  / preferred coding style for func tests is to
always pass the set of sub-dir/file components to scratch_file(),
and never try to manually construct file paths using 'join'.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: should the functional-testing scratch_file() utility guarantee that the path it returns doesn't exist?

[Peter Maydell]

On Mon, 8 Sept 2025 at 14:42, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Sep 08, 2025 at 02:13:54PM +0100, Peter Maydell wrote:
> > The functional testing framework includes a utility function
> > scratch_file() which you can use to get a path for a scratch
> > file to use in the test. However, it doesn't do anything to
> > ensure that the path it returns doesn't already exist.
> > Should it?
>
> No, we need to be able to use scratch_file() to refer to
> previously created resources. For example, when we extract
> files from an archive, we want to then reference those, or
> when we uncompress a file.
>
> > I ran into this with the aarch64/test_rme_sbsaref.py test:
> > this does:
> >         rme_stack = self.scratch_file('.')
> > to get a scratch path, and then uses it both to place
> > specific files:
> >         pflash0 = join(rme_stack, 'out', 'SBSA_FLASH0.fd')
> > and to place subdirectories:
> >         efi = join(rme_stack, 'out', 'EFI')
> >         os.makedirs(efi, exist_ok=True)
> >
> > In the original version of this test we used os.mkdir(efi),
> > but this fails sometimes because the directory already exists
> > (typically if the test was interrupted during a previous run);
> > see this email for the error log:
> > https://lore.kernel.org/qemu-devel/CAFEAcA_ZQ13qMRUQsieJiEPV=ULrDbz8=EJaW4_kw=yEysob0w@mail.gmail.co/m
>
> We have a tearDown function which purges everything under the
> scratch dir location, so if a test is interrupted in a "normal"
> way we should be safe on re-run. Only if python is hard-terminated
> would the tearDown method fail to get run.

FWIW I think the cases when I ran into this were likely
after a "kill the 'make check' run by hammering ^C until it
stops".

If you run with QEMU_TEST_KEEP_SCRATCH then the test
dir will also not be purged, obviously. So the test suite
as a whole needs to be robust against the scratch dir
existing when it starts.

> Perhaps we should proactively purge the scratch location in
> the setUp method too, as a safety net against previous unclean
> shutdown, as the intent of the 'scratch file' concept is that
> the test should have a clean environment when it starts running.

This seems like a good idea.

thanks
-- PMM