| Message ID | 20211203122223.2780098-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: multiprocessing!! | expand |
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 ...) Reviewed-by: John Snow <jsnow@redhat.com>
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. Kevin
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.
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 :)
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:
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(-)