Re: [PATCH for-5.2] tests/9pfs: Mark "local" tests as "slow"

[Greg Kurz <groug@kaod.org>]

On Mon, 23 Nov 2020 20:35:58 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 23. November 2020 19:08:18 CET Christian Schoenebeck wrote:
> > On Montag, 23. November 2020 18:41:51 CET Greg Kurz wrote:
> > > The "local" tests can fail on some automated build systems as
> > > reported here:
> > > 
> > > https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
> > > 
> > > This will need to be investigated and addressed later. Let's go for a
> > > workaround in the meantime : mark the "local" tests as "slow" so that
> > > they aren't executed with a simple "make check" like in the case above.
> > > 
> > > Reported-by: Cole Robinson <crobinso@redhat.com>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > 
> > > Unless there's a strong argument against merging this, I'll post
> > > a PR with this fix for RC3.
> > > ---
> > > 
> > >  tests/qtest/virtio-9p-test.c |   35 +++++++++++++++++++++--------------
> > >  1 file changed, 21 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > > index 21e340fa5f43..dad37ace9772 100644
> > > --- a/tests/qtest/virtio-9p-test.c
> > > +++ b/tests/qtest/virtio-9p-test.c
> > > @@ -1454,20 +1454,27 @@ static void register_virtio_9p_test(void)
> > > 
> > >      qos_add_test("synth/readdir/split_128", "virtio-9p",
> > >      
> > >                   fs_readdir_split_128,  &opts);
> > > 
> > > -
> > > -    /* 9pfs test cases using the 'local' filesystem driver */
> > > -    opts.before = assign_9p_local_driver;
> > > -    qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
> > > -    qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);
> > > -    qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir,
> > > &opts); -    qos_add_test("local/create_file", "virtio-9p",
> > > fs_create_file,
> > > &opts); -    qos_add_test("local/unlinkat_file", "virtio-9p",
> > > fs_unlinkat_file, &opts); -    qos_add_test("local/symlink_file",
> > > "virtio-9p", fs_symlink_file, &opts); -
> > > qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink, -
> > > 
> > >                &opts);
> > > 
> > > -    qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file,
> > > &opts); -    qos_add_test("local/unlinkat_hardlink", "virtio-9p",
> > > fs_unlinkat_hardlink, -                 &opts);
> > > +    if (g_test_slow()) {
> > 
> > If you don't mind, I would suggest simply using
> > 
> > if (!g_test_slow()) {
> > 	return;
> > }
> > 
> > here to keep the diff noise low.
> > 
> > > +        /* 9pfs test cases using the 'local' filesystem driver */
> > > +        /*
> > > +         * XXX: Until we are sure that these tests can run everywhere,
> > > +         * keep them as "slow" so that they aren't run with "make check"
> > > +         */
> > > +        opts.before = assign_9p_local_driver;
> > > +        qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
> > > +        qos_add_test("local/create_dir", "virtio-9p", fs_create_dir,
> > > &opts); +        qos_add_test("local/unlinkat_dir", "virtio-9p",
> > > fs_unlinkat_dir, &opts); +        qos_add_test("local/create_file",
> > > "virtio-9p", fs_create_file, &opts); +
> > > qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, +
> > > 
> > >              &opts);
> > > 
> > > +        qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file,
> > > &opts); +        qos_add_test("local/unlinkat_symlink", "virtio-9p",
> > > fs_unlinkat_symlink, +                     &opts);
> > > +        qos_add_test("local/hardlink_file", "virtio-9p",
> > > fs_hardlink_file,
> > > +                     &opts);
> > > +        qos_add_test("local/unlinkat_hardlink", "virtio-9p",
> > > +                     fs_unlinkat_hardlink, &opts);
> > > +    }
> > > 
> > >  }
> > >  
> > >  libqos_init(register_virtio_9p_test);
> > 
> > I agree that this is okay for now to avoid inconveniences with nearby 5.2
> > release, but do we agree that this is going to be reverted when 6.0
> > development phase starts?
> > 
> > In 6.0 I would rather handle this by introducing a previously mentioned
> > 'loglevel' option for 9p, and asking for the required log data if the local
> > tests fail for somebody. I can take care about the loglevel option when 6.0
> > phase starts.
> > 
> > Best regards,
> > Christian Schoenebeck
> 
> BTW, if I had to shoot in the dark, then my guess would be that system where 
> mkdir() failed with ENOTSUPP, does not support xattrs.
> 
> If that's the root cause, then another option would be skipping the 'local' 9p 
> tests if the host system lacks xattr support, instead of disabling these tests 
> for all people by default.
> 

Maybe but it's a bit too late now. RC3 is later today. I really must
post something that fixes the "make check" breakage this morning.

> Best regards,
> Christian Schoenebeck
> 
>