r-lib / testthat

An R 📦 to make testing 😀
https://testthat.r-lib.org
Other
887 stars 317 forks source link

disallow empty strings for test names #1980

Open kevinushey opened 3 weeks ago

kevinushey commented 3 weeks ago

Currently, testthat accepts empty strings as test names, but this causes issues with snapshot tests. For example:

test_that("", {
  expect_snapshot(1 + 1)
})

But every time this test file is run, testthat doesn't recognize the existing snapshot file, and so always prints:

ℹ Testing example
✔ | F W  S  OK | Context
✔ |   1      1 | hello                                                        
──────────────────────────────────────────────────────────────────────────────
Warning (test-hello.R:3:4): 
Adding new snapshot:
Code
  1 + 1
Output
  [1] 2
──────────────────────────────────────────────────────────────────────────────

══ Results ═══════════════════════════════════════════════════════════════════
[ FAIL 0 | WARN 1 | SKIP 0 | PASS 1 ]

One user encountered this during the package development workshop at posit::conf, so learners might unintentionally fall into this trap.

hadley commented 3 weeks ago

Could/should we just make an empty test name an error unconditionally? (or maybe make it a warning and then upgrade it to an error in a couple of years)

kevinushey commented 3 weeks ago

We could, but there's a lot of tests in testthat itself which also use empty names, so I wasn't sure if that was intentionally allowed.

hadley commented 3 weeks ago

Hmmm yeah, mostly used for testing though. I think we could fix those.