Re: [PATCH v2 1/8] tests/qemu-iotests/testrunner: Allow parallel test invocations

[Kevin Wolf <kwolf@redhat.com>]

Am 11.02.2022 um 17:14 hat Hanna Reitz geschrieben:
> On 11.02.22 17:00, Kevin Wolf wrote:
> > Am 11.02.2022 um 14:53 hat Thomas Huth geschrieben:
> > > On 11/02/2022 10.29, Kevin Wolf wrote:
> > > > Am 09.02.2022 um 11:15 hat Thomas Huth geschrieben:
> > > > > If multiple tests run in parallel, they must use unique file
> > > > > names for the test output.
> > > > > 
> > > > > Suggested-by: Hanna Reitz <hreitz@redhat.com>
> > > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > > ---
> > > > >    tests/qemu-iotests/testrunner.py | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> > > > > index 0eace147b8..9d20f51bb1 100644
> > > > > --- a/tests/qemu-iotests/testrunner.py
> > > > > +++ b/tests/qemu-iotests/testrunner.py
> > > > > @@ -259,7 +259,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
> > > > >            """
> > > > >            f_test = Path(test)
> > > > > -        f_bad = Path(f_test.name + '.out.bad')
> > > > > +        f_bad = Path(f'{os.getpid()}-{f_test.name}.out.bad')
> > > > >            f_notrun = Path(f_test.name + '.notrun')
> > > > >            f_casenotrun = Path(f_test.name + '.casenotrun')
> > > > >            f_reference = Path(self.find_reference(test))
> > > > Hmm... It does make sense, but nobody ever cleans those files up.
> > > > Currently, the next run of the test will just overwrite the existing
> > > > file or delete it when the test succeeds. So after running the test
> > > > suite, you have .out.bad files for every failed test, but not for those
> > > > that succeeded.
> > > > 
> > > > After this change, won't the test directory accumulate tons of .out.bad
> > > > files over time?
> > > True ... but we certainly want to keep the file for failed tests for further
> > > analysis instead of immediately deleting them ... maybe it would be enough
> > > to encode the image format (qcow2, qed, vmdk, ...) into the output name,
> > > instead of using the PID, so that "make check SPEED=thorough" works as
> > > expected here?
> > It depends on what the supported use case for test suites running in
> > parallel is. If it's just for testing multiple formats at the same time,
> > then this would work, yes.
> > 
> > I could think of more test runs that you might want to do in parallel,
> > like different protocols, different image format options, maybe even
> > different host file system. I'm not sure if all (or any) of these are
> > relevant, though.
> > 
> > Supporting only things that "make check" uses might be a good
> > compromise.
> 
> Personally and originally, I wrote that diff to allow me to actually run the
> very same test many times in parallel.  If an error occurs only very rarely,
> then I like running like 24 loops of the same test with exactly the same
> configuration (just different TEST_DIRs, of course) in parallel.
> 
> The fact that the .out.bad files tend to accumulate is why I haven’t sent it
> upstream so far.  Personally, I like Vladimir’s idea to put them into
> TEST_DIR, but probably just because this works so well for my usual case
> where TEST_DIR is on tmpfs and I thus don’t have to clean it up.

I think it could actually work fine because if you don't override
TEST_DIR, it's the same every time, and then you get the old behaviour,
just with the .out.bad files moved into scratch/.

Kevin