Gather all failures in roxygen_test.R

[MichaelChirico]

Closes #2931. Co-written with Gemini.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.23%. Comparing base (e6b2834) to head (0a0da18).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3012   +/-   ##
=======================================
  Coverage   99.23%   99.23%           
=======================================
  Files         128      128           
  Lines        7288     7288           
=======================================
  Hits         7232     7232           
  Misses         56       56           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Bisaloo]

Thanks, I think it's a helpful change! I left two minor comments.

Would it be worth testing it in another PR before merging?

[Bisaloo]

The switch between looking at "what failed" in check_roxygenize_idempotent() vs "what passed" here is not the most straightforward to resonate about IMO.

[MichaelChirico]

Yea I think that's me being too cute to save 2 lines.

[Bisaloo]

No, sorry, I wasn't clear. I think what you changed was perfectly reasonable.

I highlighted the fact we use any_failed inside check_roxygenize_idempotent() to all_locales_passed outside. It requires some mental gymnastics when thinking about the code, to make the boolean negation.

I would use all_passed (rather than any_failed) inside check_roxygenize_idempotent() or any_locale_failed in the for loop here.

Does this make sense?

(More generally, and slightly off-topic, I would likely generally consider that using ! in an return() is likely a code smell for readability. Maybe a linter could make sense.)

[MichaelChirico]

Would it be worth testing it in another PR before merging?

Good idea! I'd tested locally, but it's good for transparency to have it in the public logs. #3013