Skip to content

Skip coverage when launching parallel processes #2212

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

Merged
merged 1 commit into from
Aug 13, 2025
Merged

Conversation

hadley
Copy link
Member

@hadley hadley commented Aug 13, 2025

No description provided.

Copy link

claude bot commented Aug 13, 2025

Claude finished @hadley's task —— View job


Code Review Progress

  • Read changed files to understand modifications
  • Analyze changes for potential bugs or issues
  • Provide review feedback

Review

The changes add skip_on_covr() calls to parallel testing functions that involve spawning processes. This prevents these tests from running during code coverage analysis, which is appropriate since:

  1. Process spawning tests can interfere with coverage collection
  2. Similar pattern already exists in other parallel tests in the codebase
  3. The teardown test was already skipped anyway due to the skip("teardown errors are ignored") call

LGTM - these are defensive test skips that prevent coverage measurement interference without affecting actual test functionality.


@hadley
Copy link
Member Author

hadley commented Aug 13, 2025

@claude did I miss any tests?

Copy link

claude bot commented Aug 13, 2025

Claude finished @hadley's task —— View job


Analysis: Checking for missed parallel tests

  • Search for other parallel test files
  • Check for process spawning tests
  • Review existing skip_on_covr() usage
  • Analyze findings and provide recommendations

Findings

Good news - you didn't miss any tests! All parallel test files that spawn processes and could interfere with coverage collection already have skip_on_covr():

Files already protected (before this PR):

  • test-parallel-crash.R:3 - ✅ Has skip_on_covr()
  • test-parallel-errors.R:2 - ✅ Has skip_on_covr()
  • test-parallel-setup.R:2 - ✅ Has skip_on_covr()
  • test-parallel-startup.R:2 - ✅ Has skip_on_covr()
  • test-parallel-stdout.R:2 - ✅ Has skip_on_covr()

Files covered by this PR:

  • test-parallel-outside.R - ✅ Added skip_on_covr()
  • test-parallel-teardown.R - ✅ Added skip_on_covr()
  • test-parallel.R - ✅ Added skip_on_covr() to process-spawning tests

Non-issues:

  • test-reporter-progress.R only has ParallelProgressReporter tests (no process spawning)

All parallel tests that spawn subprocesses via test_local() with TESTTHAT_PARALLEL = "TRUE" are now properly protected from coverage interference.


@hadley hadley changed the title Maybe it's testing parallelism within the tests? Skip coverage when launching parallel processes Aug 13, 2025
@hadley
Copy link
Member Author

hadley commented Aug 13, 2025

A test-coverage job ran successfully three times in a row, so I think this is the fix.

@hadley hadley closed this Aug 13, 2025
@hadley hadley reopened this Aug 13, 2025
@hadley hadley merged commit 132a2ff into main Aug 13, 2025
27 checks passed
@hadley hadley deleted the parallel-tests-covr branch August 13, 2025 16:37
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.

1 participant