Additional tests for attributes #1446
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive test coverage for the Rcpp attributes system to increase code coverage and remove #nocov tags from previously untested code paths. The tests target specific error conditions and parameter variations in the attributes functionality.
Changes:
- Added four new test files covering error conditions and attribute parameter variations
- Removed
#nocovtags fromsrc/attributes.cppandR/Attributes.Rwhere new tests now provide coverage - Retained
#nocovtags for platform-specific code that cannot be tested under Linux coverage analysis
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/attributes.cpp | Removed #nocov tags from exportedName, exportedCppName, rng, and invisible methods now covered by tests |
| R/Attributes.R | Removed #nocov tags from error paths and parameter validation, kept tags for Windows-specific code |
| inst/tinytest/test_sourcecpp_errors.R | New tests for sourceCpp error conditions (invalid extensions, filenames with spaces, windowsDebugDLL) |
| inst/tinytest/test_cppfunction_errors.R | New tests for cppFunction error conditions (no function, multiple functions) |
| inst/tinytest/test_compile_attributes_errors.R | New tests for compileAttributes error conditions (missing DESCRIPTION/NAMESPACE) |
| inst/tinytest/test_attributes_extended.R | New tests for attribute parameter variations (named exports, RNG, invisible) |
| inst/tinytest/cpp/attributes_extended.cpp | C++ test code with various Rcpp export attribute configurations |
| ChangeLog | Documents the addition of new test files and removal of #nocov tags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## compilation dies sooner so we never actually see the messages | ||
| expect_error(cppFunction(code)) #, "More than one function definition") |
There was a problem hiding this comment.
The test for multiple function definitions (line 328 in R/Attributes.R) does not actually verify the intended error message. The comment on line 34 indicates compilation fails before reaching this check, so the #nocov tag should remain on line 328 of R/Attributes.R. Either enhance the test to properly trigger the error condition or revert the #nocov removal.
There was a problem hiding this comment.
I think this has already been done
| if (grepl(' ', basename(file), fixed=TRUE)) { # #nocov start | ||
| stop("The filename '", basename(file), "' contains spaces. This ", | ||
| "is not permitted.") | ||
| "is not permitted.") # #nocov end |
There was a problem hiding this comment.
The indentation uses tabs instead of spaces. The codebase appears to use spaces for indentation. Consider using consistent spacing (tabs should generally be avoided in favor of spaces).
| "is not permitted.") # #nocov end | |
| "is not permitted.") # #nocov end |
| ## Coverage target: R/Attributes.R:58-61 | ||
| tmpfile <- tempfile(fileext = ".c") | ||
| writeLines("int main() { return 0; }", tmpfile) | ||
| expect_error(sourceCpp(tmpfile), "does not have an extension of .cc or .cpp") |
There was a problem hiding this comment.
The error message pattern uses '.cc' which should be '\.cc' to properly escape the dot in regex matching. The same applies to '.cpp'. While this may work due to partial matching, it's technically incorrect as '.' matches any character in regex.
| expect_error(sourceCpp(tmpfile), "does not have an extension of .cc or .cpp") | |
| expect_error(sourceCpp(tmpfile), "does not have an extension of \\.cc or \\.cpp") |
| expect_equal(test_rng_lowercase_true(), 789) | ||
| expect_equal(test_rng_uppercase_true(), 101) | ||
| expect_equal(test_rng_lowercase_false(), 202) |
There was a problem hiding this comment.
These tests verify that functions with different RNG parameter values execute successfully, but they don't actually verify that the RNG behavior differs. The tests only check return values, not whether RNG state management is correctly applied based on the parameter.
This pull request is a bit of an experiment in that I asked Claude Sonnet 4.5 (via the Google Antigravity IDE, another Code clone) to examine existing
#nocovtags and fill in some tests. And so it did which we can now look at in more detail.It is all fairly reasonable if pedestrian (i.e. checking on Windows for permitted filenames etc) but does exactly what it was asked: increase coverage of tests across the codebase by a bit. If we like this we can repeat as this touched only one C++ and one R file so far (for attributes, in this case).
I did one minimal round of postprocessing so far. In one case we still need the
#nocov: the windows-specific code will never run under Linux coverage analysis. And one expected error does not materialize as the compilation fails (cannot have two functions incppFunction()call).Checklist
R CMD checkstill passes all tests