Skip to content

Conversation

@MichaelChirico
Copy link
Collaborator

Closes #2292

@codecov
Copy link

codecov bot commented Mar 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.64%. Comparing base (d590594) to head (fc20fed).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2808   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files         127      127           
  Lines        7068     7078   +10     
=======================================
+ Hits         7043     7053   +10     
  Misses         25       25           

☔ 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.

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly I found two odd usages of test_that on GitHub https://github.com/search?q=%2Ftest_that%5C(%5C%7B%2F&type=code

  1. Piping the test name.
  2. Supplying the description as a keyword argument.

Both break the positional assumption.

WDYT about supporting one or both?

@MichaelChirico
Copy link
Collaborator Author

Interestingly I found two odd usages of test_that on GitHub https://github.com/search?q=%2Ftest_that%5C(%5C%7B%2F&type=code

  1. Piping the test name.
  2. Supplying the description as a keyword argument.

Both break the positional assumption.

WDYT about supporting one or both?

I don't think it's worth the investment. If eventually we have a good general framework for handling odd cases like that, we can include it in a sweep of all linters. But I also don't see much demand for that besides from us maintainers recognizing we're being less-than-perfect on this front.

I see 28 hits there vs. >300,000 for just test_that[(]['"], and at least one of them is Lionel presumably himself doing some edge case testing :)

MichaelChirico and others added 2 commits March 3, 2025 23:32
Co-authored-by: AshesITR <alexander.rosenstock@web.de>
@IndrajeetPatil IndrajeetPatil requested a review from AshesITR April 23, 2025 13:24
@MichaelChirico
Copy link
Collaborator Author

Bumping for review :)

Copy link
Collaborator

@olivroy olivroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idea + tests look good to me!

wonder if this will catch this

obrl-soil/h3jsr#11 (see what I did to tests here.

test_that("test", x<-1. expect_equal(x, 1), expect_equal(x + 1,2))

@MichaelChirico
Copy link
Collaborator Author

Sorry, I'm not sure what I'm looking at 😅 Your example doesn't parse, and the PR you shared has too much other things going on :)

@MichaelChirico MichaelChirico merged commit b9c4205 into main Jun 24, 2025
20 checks passed
@MichaelChirico MichaelChirico deleted the test-that-braces branch June 24, 2025 18:00
@olivroy
Copy link
Collaborator

olivroy commented Jun 25, 2025

Sorry, I typed too fast on my phone. See this view instead https://github.com/obrl-soil/h3jsr/pull/11/files?file-filters%5B%5D=.R&show-deleted-files=true&show-viewed-files=true

@MichaelChirico
Copy link
Collaborator Author

Can't say I've seen that one before 🫨

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.

Encourage braced expression in test_that() calls

4 participants