Re: [PATCH 2/3] iotests/testrunner.py: move updating last_elapsed to run_tests

[Vladimir Sementsov-Ogievskiy]

10.12.2021 17:47, Vladimir Sementsov-Ogievskiy wrote:
> 10.12.2021 17:25, Kevin Wolf wrote:
> > Am 06.12.2021 um 18:59 hat John Snow geschrieben:
> > > On Fri, Dec 3, 2021 at 7:22 AM Vladimir Sementsov-Ogievskiy <
> > > vsementsov@virtuozzo.com> wrote:
> > >
> > > > We are going to use do_run_test() in multiprocessing environment, where
> > > > we'll not be able to change original runner object.
> > > >
> > > > Happily, the only thing we change is that last_elapsed and it's simple
> > > > to do it in run_tests() instead. All other accesses to self in
> > > > do_runt_test() and in run_test() are read-only.
> > > >
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > > ---
> > > >   tests/qemu-iotests/testrunner.py | 4 +++-
> > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tests/qemu-iotests/testrunner.py
> > > > b/tests/qemu-iotests/testrunner.py
> > > > index fa842252d3..a9f2feb58c 100644
> > > > --- a/tests/qemu-iotests/testrunner.py
> > > > +++ b/tests/qemu-iotests/testrunner.py
> > > > @@ -287,7 +287,6 @@ def do_run_test(self, test: str) -> TestResult:
> > > >                                 diff=diff, casenotrun=casenotrun)
> > > >           else:
> > > >               f_bad.unlink()
> > > > -            self.last_elapsed.update(test, elapsed)
> > > >               return TestResult(status='pass', elapsed=elapsed,
> > > >                                 casenotrun=casenotrun)
> > > >
> > > > @@ -353,6 +352,9 @@ def run_tests(self, tests: List[str]) -> bool:
> > > >                       print('\n'.join(res.diff))
> > > >               elif res.status == 'not run':
> > > >                   notrun.append(name)
> > > > +            elif res.status == 'pass':
> > > > +                assert res.elapsed is not None
> > > > +                self.last_elapsed.update(t, res.elapsed)
> > > >
> > > >               sys.stdout.flush()
> > > >               if res.interrupted:
> > > > --
> > > > 2.31.1
> > > (I continue to be annoyed by the "None" problem in mypy, but I suppose it
> > > really can't be helped. Nothing for you to change with this patch or
> > > series. I just wish we didn't need so many assertions ...)
> >
> > I'm inclined to say it's a None problem in our code, not in mypy.
> > Essentially it comes from the fact that we're abusing a string
> > (res.status) and None values to distinguish different types of results
> > that have a different set of valid attributes.
> >
> > Of course, Python already provides a language feature to distinguish
> > different types of results that have a different set of attributes and
> > that wouldn't run into this problem: subclasses.
>
> Agree, you are right. I'll look if it is simple to refactor.

No it's not simple)

Actually, it means making TestResult more smart, and moving (most of) logic of test result 
result parsing (checking different files, etc) to TestResult.. But we'll still want to update 
last_elapsed.. Adding a method "TestResult.update_runnner(runner)", which will be 
no-op in base TestResult and will be actually realized only in TestResult subclass that have 
.elapsed to call runner.update_last_elapsed()? And we'll have a hierarhy like TestResultBase 
-> TestResultWithElapsed -> (TestResultFail, TestResultPass).. At least, that's what I 
imagine, and I don't like it :)

--
Best regards,
Vladimir