Skip to content

Conversation

@hadley
Copy link
Member

@hadley hadley commented Oct 8, 2025

We should consider if it's worth doing this automatically in the CheckReporter.

  • Save in tests/testthat/problems (as suggested by Kurt).
  • Some way to disable with an env var in case they turn out to be annoying.
  • Add library(testthat) and load package somehow? Needs to be inside namespace

@claude
Copy link

claude bot commented Oct 8, 2025

Claude finished @hadley's task —— View job


PR Review

Todo List:

  • Read CLAUDE.md for repo guidelines
  • Review R/extract.R implementation
  • Review documentation and other changes
  • Provide feedback on potential bugs/issues

Issues Found

R/extract.R:21 - Potential bug with test_path() usage. The function expects to resolve test file paths, but test_path(pieces[[1]]) may not work as intended. test_path() is designed for finding files within tests/testthat directory during testing, but here it's being used on what appears to be a file path from a location string like "test-foo.R:10". This could fail if the file doesn't exist in the current working context.

R/extract.R:59 - Logic assumes at least one subtest exists but doesn't validate this. If is_subtest returns all FALSE, which(is_subtest) returns integer(0) and rev(which(is_subtest))[[1]] will error.

R/extract.R:87 - Typo in error message: "test call has use {" should be "test call must use {".

R/extract.R:68 - Hard-coded path prefix "tests/testthat/" may not match actual file location when test_path() modifies the input path.

Minor Issues

Documentation has typo: "Extract a reprex from an failed expectation" should be "a failing expectation".

No test file exists for this new functionality, violating the repo's requirement that all new code should have accompanying tests.

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