RFC: fail `test.data.table()` if non-test code produces warnings #7210

aitap · 2025-07-24T19:59:53Z

Related to #7209. I happened to notice the warning while looking at unrelated failures. Should test.data.table() explicitly go looking for warnings produced by code outside the test() calls and fail R CMD check because of them?

Pros: sometimes there's a good reason to fix the warning (like #7209).

Cons: sometimes warnings happen for stupid reasons outside our direct control. For example, a system that neither currently has a UTF-8 locale set nor has en_US.UTF-8 installed will cause a warning between tests 2252.2 and 2253.01, because the setlocale() call will fail:

https://github.com/Rdatatable/data.table/blob/63339e1bf4a65b1821c0c5a7019270d34e63346e/inst/tests/tests.Rraw#L18619-L18623

github-actions · 2025-07-24T20:19:18Z

HEAD=test-warnings slower P<0.001 for memrecycle regression fixed in #5463

Generated via commit 3ddef1a

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	2 minutes and 55 seconds
Installing different package versions	59 seconds
Running and plotting the test cases	2 minutes and 52 seconds

MichaelChirico · 2025-07-24T20:30:45Z

Personally I like requiring warnings to be handled properly everywhere, I'm a fan of setting options(warn=2) in my own {testthat} suites, for example.

If it's really a stupid/spurious warning, just suppressWarnings() and move on.

That setlocale() example is a case in point -- we should maybe skip some tests where we assume we're allowed to change locales, but can't, that could have unanticipated results.

aitap · 2025-07-26T12:23:54Z

inst/tests/tests.Rraw

+local(if (l10n_info()$`UTF-8` || {
  lc_ctype = Sys.getlocale('LC_CTYPE')
+  lc_wantctype = 'en_US.UTF-8'
  # Japanese multibyte characters require utf8. As of 2025, we're likely to be already running in a UTF-8 locale, but if not, try this setlocale() call as a last chance.
  # Unfortunately, there is no guaranteed, portable way of switching to UTF-8 US English.
-  if (!l10n_info()$`UTF-8`) Sys.setlocale('LC_CTYPE', "en_US.UTF-8")
-  on.exit(Sys.setlocale('LC_CTYPE', lc_ctype))
+  lc_newctype = suppressWarnings(Sys.setlocale('LC_CTYPE', lc_wantctype))
+  if (identical(lc_newctype, lc_wantctype)) {
+    on.exit(Sys.setlocale('LC_CTYPE', lc_ctype))
+    TRUE
+  } else FALSE
+}) {


Seeking suggestions for how to improve this test. The logic is as follows:

If l10n_info()$`UTF-8`, that's fine, proceed with the tests.

Otherwise, try to set the UTF-8 charset from the en_US.UTF-8 locale.

If succeeded, use on.exit() to restore the locale afterwards and proceed with the tests.

Otherwise, skip the tests.

The current code works, but putting if with side effects inside another if doesn't feel like a good idea.

it's ugly, but it looks sound.

if anything, maybe we would just move it inside test.data.table() like foreign:

data.table/R/test.data.table.R

Lines 119 to 125 in 39e2875

foreign = gettext("object '%s' not found", domain="R") != "object '%s' not found"

if (foreign) {

# nocov start

catf("\n**** This R session's language is not English. Each test will still check that the correct number of errors and/or\n**** warnings are produced. However, to test the text of each error/warning too, please restart R with LANGUAGE=en\n\n")

# nocov end

}

assign("foreign", foreign, envir=env)

Then we just do, say, if (supports_en_utf) local(...).

It would also facilitate if this sort of check would make other tests easier, we don't need to duplicate this complicated logic more than once.

I think it would be simpler but no less reliable if we skipped these tests when an enc2native() check fails, but I opened #7217 because there are other tests in a similar position, outside the scope of this PR.

On modern versions of R for Windows, Sys.setlocale() warns for non-UTF-8 locales. Also comment the previous instance of suppressWarnings().

MichaelChirico · 2025-07-27T00:06:50Z

R/test.data.table.R

+        # nocov start
+        if (!silent && showProgress) print(w)
+        env$warnings = c(env$warnings, list(list(
+          env$prevtest, toString(w),


why not conditionMessage()?

~~should we give this object a bit more structure, e.g. name the elements for clarity?~~ I see it's done below

R/test.data.table.R

* conditionMessage() instead of toString() * apply names earlier * don't check for zero-length calls in sys.calls() * be careful not to over-translate an already-translated message Co-Authored-By: Michael Chirico <michaelchirico4@gmail.com>

codecov · 2025-07-27T13:36:58Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.77%. Comparing base (ca30752) to head (3ddef1a).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7210   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files          81       81           
  Lines       15215    15237   +22     
=======================================
+ Hits        15029    15051   +22     
  Misses        186      186

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

test.data.table: catch warnings outside test code

0572c11

aitap requested a review from MichaelChirico as a code owner July 24, 2025 19:59

aitap added 2 commits July 24, 2025 23:23

Use explicit environment assignment instead of <<-

08765e1

Thinko: length -> nrow

dcea5b5

jangorecki approved these changes Jul 25, 2025

View reviewed changes

aitap added 4 commits July 26, 2025 11:34

tests 2253*: suppressWarnings, skip if no UTF-8

8606301

tests: remember tracebacks from warnings

519c5e5

Thinko: length -> nrow

9bc3daa

Merge branch 'master' into test-warnings

4f04422

aitap commented Jul 26, 2025

View reviewed changes

suppressWarnings(Sys.setlocale(...))

69d9555

On modern versions of R for Windows, Sys.setlocale() warns for non-UTF-8 locales. Also comment the previous instance of suppressWarnings().

MichaelChirico reviewed Jul 27, 2025

View reviewed changes

R/test.data.table.R Outdated Show resolved Hide resolved

MichaelChirico reviewed Jul 27, 2025

View reviewed changes

R/test.data.table.R Show resolved Hide resolved

aitap mentioned this pull request Jul 27, 2025

Tests don't handle non-UTF-8 locales well #7217

Open

Apply fixes from code review

3ddef1a

* conditionMessage() instead of toString() * apply names earlier * don't check for zero-length calls in sys.calls() * be careful not to over-translate an already-translated message Co-Authored-By: Michael Chirico <michaelchirico4@gmail.com>

MichaelChirico approved these changes Jul 28, 2025

View reviewed changes

MichaelChirico merged commit 1e38fc2 into master Jul 28, 2025
12 checks passed

MichaelChirico deleted the test-warnings branch July 28, 2025 17:45

aitap mentioned this pull request Sep 15, 2025

GitLab CI: install XQuartz on macOS #7319

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: fail `test.data.table()` if non-test code produces warnings #7210

RFC: fail `test.data.table()` if non-test code produces warnings #7210

Uh oh!

aitap commented Jul 24, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jul 24, 2025 •

edited

Loading

Uh oh!

MichaelChirico commented Jul 24, 2025 •

edited

Loading

Uh oh!

aitap Jul 26, 2025

Uh oh!

MichaelChirico Jul 27, 2025 •

edited

Loading

Uh oh!

aitap Jul 27, 2025

Uh oh!

MichaelChirico Jul 27, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 27, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	foreign = gettext("object '%s' not found", domain="R") != "object '%s' not found"
	if (foreign) {
	# nocov start
	catf("\n** This R session's language is not English. Each test will still check that the correct number of errors and/or\n** warnings are produced. However, to test the text of each error/warning too, please restart R with LANGUAGE=en\n\n")
	# nocov end
	}
	assign("foreign", foreign, envir=env)

RFC: fail test.data.table() if non-test code produces warnings #7210

RFC: fail test.data.table() if non-test code produces warnings #7210

Uh oh!

Conversation

aitap commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelChirico commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aitap Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

MichaelChirico Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aitap Jul 27, 2025

Choose a reason for hiding this comment

Uh oh!

MichaelChirico Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RFC: fail `test.data.table()` if non-test code produces warnings #7210

RFC: fail `test.data.table()` if non-test code produces warnings #7210

aitap commented Jul 24, 2025 •

edited

Loading

github-actions bot commented Jul 24, 2025 •

edited

Loading

MichaelChirico commented Jul 24, 2025 •

edited

Loading

MichaelChirico Jul 27, 2025 •

edited

Loading

MichaelChirico Jul 27, 2025 •

edited

Loading

codecov bot commented Jul 27, 2025 •

edited

Loading