gh-109817: Add --single-process-per-case option to libregrtest - #151689
Conversation
|
i'll fix these |
|
I'm a little busy now, but I'll review it after finishing with other unittest/regrtest tasks. Thank you for your efforts. |
…ase in its own worker process
no hurry, i'm improving this till then, so hopefully it creates less pain for you to review 😊 . I've added tests, with some improvement (locally everything works fine for me) . i'll once again go through this and check for improvement areas before you come here . |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Thank you for working on this.
It looks like the code does what the issue asked. But I suspect it can be simplified.
| @@ -94,19 +94,47 @@ def list_cases(tests: TestTuple, *, | |||
| test_dir: StrPath | None = None) -> None: | |||
| support.verbose = False | |||
| set_match_tests(match_tests) | |||
There was a problem hiding this comment.
set_match_tests() is called both in list_cases() and in collect_cases().
| with self.lock: | ||
| self.tests_iter = None | ||
|
|
||
| class GroupedMultiprocessIterator: |
There was a problem hiding this comment.
Cannot MultiprocessIterator be used instead? These classes look very similar.
| finally: | ||
| self.test_name = _NOT_RUNNING | ||
|
|
||
| mp_result = dataclasses.replace( |
There was a problem hiding this comment.
mp_result.result is mutable, it can be mutated in-place (see _run_flat()).
| finally: | ||
| self.output.put(WorkerThreadExited()) | ||
|
|
||
| def _run_grouped(self) -> None: |
There was a problem hiding this comment.
_run_grouped() looks very similar to _run_flat() (especially if unify iterators). Could not the share the parametrized body?
thanks for the review , i'll update shortly with the improvement points you shared . |
|
@serhiy-storchaka gentle ping, i've updated with changes, please have a look in this when you get chance and let me know if this needs any further improvement. Thankyou :) |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Thank you, this is much simpler now.
collect_cases() skips unittest.loader._FailedTest. This was harmless while it only fed --list-cases, but it now decides what actually runs: a module which fails to import produces no cases and silently disappears. Where a normal run reports FAILURE and returns 2, --single-process-per-case reports SUCCESS and returns 0. _FailedTest should be reported rather than skipped.
The skipped list returned by collect_cases() is discarded in _run_tests(), so modules which raise SkipTest on import are not reported as skipped.
Please add a NEWS entry in Misc/NEWS.d/next/Tests/.
I've updated with some changes to fix this, actually i started with different approach , but i asked claude to review and got suggested with this approach, please let me know your thoughts on this , i'd love to hear how you would have handled this case or there is another approach you suggest for this. I've also added news entry.. @serhiy-storchaka Thanks again for giving time in this for the review , hoping we're about to make this work ready. |
| self.test_name = _NOT_RUNNING | ||
| mp_result.result.duration = time.monotonic() - self.start_time | ||
| if single_process_per_case: | ||
| if single_process_per_case and test_name != module_name: |
There was a problem hiding this comment.
Why and test_name != module_name was added?
There was a problem hiding this comment.
it was added like for when a module has no collected test case, the group is (module, (module,)) and we run whole module, so there's no case name to report.
There was a problem hiding this comment.
ah but it looks like in that case assignment will have no effect, and this should be removed .
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
LGTM. 👍
Thank you for your contribution @Aniketsy.
2 participants
fixes #109817