Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When testing in parallel, ignore orphaned child pids. #112

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mvsjes2
Copy link

@mvsjes2 mvsjes2 commented Apr 1, 2024

Addresses issue #113

When running prove without this fix, you get "Attempt to detach from wrong child" from Test2::Async while the test suite is running, and "Subtest can only be finished in the process/thread that created it" during destruction.

With this fix, we just get a new warning "Ignoring unknown child pid $pid, did a grandchild pid die?" and no other further complaints from Test2 or Test::Class::Moose.

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 93.50%. Comparing base (f263d2e) to head (20f3a16).

Files Patch % Lines
lib/Test/Class/Moose/Executor/Parallel.pm 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   93.57%   93.50%   -0.08%     
==========================================
  Files          92       92              
  Lines        3006     3000       -6     
  Branches      142      143       +1     
==========================================
- Hits         2813     2805       -8     
- Misses        137      138       +1     
- Partials       56       57       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mvsjes2 mvsjes2 marked this pull request as draft April 5, 2024 19:21
@mvsjes2 mvsjes2 force-pushed the mvsjes2/parallel-fork-grandchild-dies branch from 31f2282 to 20f3a16 Compare April 5, 2024 20:21
@autarch
Copy link
Member

autarch commented Apr 7, 2024

Thanks for working on this! Can you also add a test for this issue that fails without the changes in this PR?

@mvsjes2
Copy link
Author

mvsjes2 commented Apr 7, 2024 via email

@autarch
Copy link
Member

autarch commented Apr 7, 2024

Yeah, unfortunately writing tests for test tools can be a bit confusing. It uses the intercept sub from https://metacpan.org/pod/Test2::API to capture all the test events generated when Test::Class::Moose runs.

Then it uses https://metacpan.org/pod/Test2::V0 to import a bunch of test helpers which are used to check the events. I think the easiest approach is to write some code that causes the bug, capture the events with intercept, dump them with something like Data::Dumper or DDP, then do the same with the bug fixed.

The "diff" between those two events is the thing you want to check for.

@mvsjes2
Copy link
Author

mvsjes2 commented Apr 7, 2024

OK, thanks for the insight. I do have a test 1/2 written for this, just not ready to push up yet. I have this PR in draft mode for now. Will switch it to ready once I think I have all the bits worked out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants