Skip to content

Handle errors in parallel teardown files #2210

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gaborcsardi
Copy link
Member

I also changed the exit status of the subprocesses to 0 (normal exit),
from 1. I didn't find any reason why it was 1, hopefully there
wasn't any. It is a normal exit, so 0 makes more senset to me (now).

Closes #1165.

I also changed the exit status of the subprocesses to 0 (normal exit),
from 1. I didn't find any reason why it was 1, hopefully there
wasn't any. It is a normal exit, so 0 makes more senset to me (now).

Closes #1165.
Copy link

claude bot commented Aug 13, 2025

Claude finished @gaborcsardi's task —— View job


Code Review

  • Read modified files to understand changes
  • Analyze test failure and its relationship to the changes
  • Identify potential bugs or issues
  • Provide code review feedback

Key Issues:

  1. Test failure appears unrelated to your changes - The test failure shows "R session closed the process connection, killed" but your teardown error collection logic should handle this properly.

  2. Exit status change from 1 to 0 - R/parallel.R:319 - This change makes sense for normal teardown exits, but ensure teardown errors still get properly surfaced through the new error collection mechanism.

  3. Worker collection fix looks correct - R/parallel.R:330 - The change from collecting connections to collecting workers is correct since you need both the connection (for polling) and the worker object (for reading messages).

The teardown error collection improvement should handle the test failure case better. The failing test might need investigation of whether the teardown error is occurring before your error collection logic can capture it.

Recommendation: Run the specific failing test in isolation to see if the error collection is working as expected, or if there's a race condition in the teardown process.

@gaborcsardi
Copy link
Member Author

Seems like this is not OK:

══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-parallel-stdout.R:16:3'): stdout/stderr in parallel code ───────
Error in `queue_teardown(queue)`: At least one parallel worker failed to run teardown
Caused by error:
! R session closed the process connection, killed
Backtrace:
     ▆
  1. ├─utils::capture.output(...) at test-parallel-stdout.R:16:3
  2. │ └─base::withVisible(...elt(i))
  3. ├─base::suppressMessages(...)
  4. │ └─base::withCallingHandlers(...)
  5. └─testthat::test_local(test_path("test-parallel", "stdout"), reporter = "progress")
  6.   └─testthat::test_dir(...) at testthat/R/test-package.R:73:3
  7.     └─testthat:::test_files(...) at testthat/R/test-files.R:88:3
  8.       └─testthat:::test_files_parallel(...) at testthat/R/test-files.R:165:5
  9.         ├─withr (local) `<fn>`() at testthat/R/parallel.R:67:3
 10.         └─testthat:::queue_teardown(queue) at testthat/R/parallel.R:54:3
 11.           └─cli::cli_abort(...) at testthat/R/parallel.R:388:5
 12.             └─rlang::abort(...)

@gaborcsardi
Copy link
Member Author

Another kind of failure:

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-parallel-crash.R:22:3'): crash ───────────────────────────────
`err` inherits from 'rlang_error'/'error'/'condition' not 'testthat_process_error'.
── Failure ('test-parallel-crash.R:23:3'): crash ───────────────────────────────
err$test_file (`actual`) is not equal to "test-crash-3.R" (`expected`).

`actual` is NULL
`expected` is a character vector ('test-crash-3.R')

[ FAIL 2 | WARN 0 | SKIP 4 | PASS 1221 ]

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

Successfully merging this pull request may close these issues.

Handle errors in parallel teardown files
1 participant