[libvirt PATCH 312/351] meson: tests: add file access test setup

Tue Jul 28 20:45:07 UTC 2020

On Tue, Jul 28, 2020 at 09:32:57PM +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 09:13:41PM +0200, Peter Krempa wrote:
> > On Tue, Jul 28, 2020 at 18:48:18 +0200, Pavel Hrdina wrote:
> > > On Tue, Jul 28, 2020 at 03:39:41PM +0200, Pavel Hrdina wrote:
> > > > On Tue, Jul 28, 2020 at 03:19:25PM +0200, Pino Toscano wrote:
> > > > > On Tuesday, 28 July 2020 14:56:45 CEST Peter Krempa wrote:
> > > > > > On Thu, Jul 16, 2020 at 11:59:08 +0200, Pavel Hrdina wrote:
> > > > > > > We need to modify check-file-access.py to be usable as wrapper for
> > > > > > > libvirt tests. This way we can run the tests using this command:
> > > > > > > 
> > > > > > >     meson test --setup access
> > > > > > > 
> > > > > > > which will run all tests using check-file-access.py as a wrapper.
> > > > > > > 
> > > > > > > With autotools all file access are written into single file for all
> > > > > > > tests and compared once the whole test suite is done.
> > > > > > > 
> > > > > > > With Meson we will compare the file access after every single test
> > > > > > > because it is used as wrapper now. That requires writing the file
> > > > > > > access into separate files for every single test as they are executed
> > > > > > > in parallel.
> > > > > > > 
> > > > > > > Since the wrapper is used for all tests in Meson including tests outside
> > > > > > > of tests directory we have to check for presence of the output file.
> > > > > > > We should also cleanup after ourselves.
> > > > > > > 
> > > > > > > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > > > > > > ---
> > > > > > >  Makefile.am                  |  3 ---
> > > > > > >  scripts/check-file-access.py | 24 +++++++++++++++++++-----
> > > > > > >  tests/Makefile.am            | 12 ------------
> > > > > > >  tests/meson.build            |  8 ++++++++
> > > > > > >  tests/virtestmock.c          |  2 +-
> > > > > > >  5 files changed, 28 insertions(+), 21 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/Makefile.am b/Makefile.am
> > > > > > > index 363c5cf66fd..d05a0c1a85a 100644
> > > > > > > --- a/Makefile.am
> > > > > > > +++ b/Makefile.am
> > > > > > > @@ -37,9 +37,6 @@ srpm: clean
> > > > > > >  
> > > > > > >  check-local: all tests
> > > > > > >  
> > > > > > > -check-access: all
> > > > > > > -	@($(MAKE) $(AM_MAKEFLAGS) -C tests check-access)
> > > > > > > -
> > > > > > >  dist-hook: gen-AUTHORS
> > > > > > >  
> > > > > > >  .PHONY: gen-AUTHORS
> > > > > > > diff --git a/scripts/check-file-access.py b/scripts/check-file-access.py
> > > > > > > index aa120cafacf..f0e98f4b652 100755
> > > > > > > --- a/scripts/check-file-access.py
> > > > > > > +++ b/scripts/check-file-access.py
> > > > > > > @@ -21,15 +21,27 @@
> > > > > > >  #
> > > > > > >  #
> > > > > > >  
> > > > > > > +import os
> > > > > > > +import random
> > > > > > >  import re
> > > > > > > +import string
> > > > > > >  import sys
> > > > > > >  
> > > > > > > -if len(sys.argv) != 3:
> > > > > > > -    print("syntax: %s ACCESS-FILE PERMITTED-ACCESS-FILE")
> > > > > > > -    sys.exit(1)
> > > > > > > +abs_builddir = os.environ.get('abs_builddir', '')
> > > > > > > +abs_srcdir = os.environ.get('abs_srcdir', '')
> > > > > > >  
> > > > > > > -access_file = sys.argv[1]
> > > > > > > -permitted_file = sys.argv[2]
> > > > > > > +filename = ''.join(random.choice(string.ascii_letters) for _ in range(16))
> > > > > > 
> > > > > > Umm, python doesn't have a 'mkostemp' equivalent? This seems a bit
> > > > > > fishy.
> > > > > 
> > > > > Sure, it is the tempfile module:
> > > > > https://docs.python.org/3/library/tempfile.html
> > > > > 
> > > > >   filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', suffix='.txt')
> > > > 
> > > > I'll look into it. When porting it I just looked for any solution as
> > > > this can be changed any time after the series is pushed.
> > > 
> > > So I looked into it and the python script doesn't create the temporary
> > > file. The file is created by virtestmock.c if we run our test suite with
> > > file access check. The existence of the file is also used to check if
> > > there is something to be compared as not all of the tests needs to be
> > > check if they access some system files, for example all the check-* test
> > > cases.
> > > 
> > > The tempfile is a nice module but useless in this case where we care
> > > about only getting a temporary file name. In order to use the module
> > > we would have to do something like this:
> > > 
> > > 
> > > fd, filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', suffix='.txt')
> > > os.close(fd)
> > > os.unlink()
> > > 
> > > 
> > > which looks ugly. So I would rather keep it as it is.
> > 
> > Your algorithm is not robust enough so it could end up causing random
> > test failures. Unlikely but possible.
> > 
> > You can pass the filename to the test and let the mock append to the
> > existing file and then read it.
> 
> You are missing the point that the presence of the file is used later in
> the script:
> 
> if ret != 0 or not os.path.exists(access_file):
>     sys.exit(ret)
> 
> the whole point is that only if the file exists the script will do the
> actual check and compare the file-access-***.txt file.
> 
> If the file doesn't exist the script will fail with:
> 
> Traceback (most recent call last):
>   File "/home/phrdina/work/libvirt/scripts/check-file-access.py", line 52, in <module>
>     with open(access_file, "r") as fh:
> FileNotFoundError: [Errno 2] No such file or directory: 'file-access-IIyVhsUpnErLEkUm.txt'
> 
> 
> If we don't mind running the whole script for test cases where the
> access check doesn't make sense the example above can be changed to
> this:
> 
> fd, filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', suffix='.txt')
> os.close(fd)
> 
> ...
> 
> if ret != 0:
>     sys.exit(ret)
> 
> but at this point it doesn't make any difference and we can just go
> with the first example where we remove the file as well.
> 
> Pavel

So I managed to make it work without closing and opening again the file
using the tempfile module. I think it's still ugly but I don't care
anymore as I want to have it ready so we can push it right after release
next week.

Just a note, the file access testing is broken and it fails for a lot of
tests.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200728/369dbe01/attachment-0001.sig>