Skip one more test on macOS

[eddelbuettel]

We are seeing an additional (new) failure on macOS and arm64 at r-universe (recent build log). We already skip a number of NA related tests on that platform so I added one. I cannot test this locally -- anybody reading this who has an arm64 mac and the clang that @jeroen reports, namely

clang --version
Apple clang version 15.0.0 (clang-1500.1.0.2.5)
Target: arm64-apple-darwin22.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

Checklist

• Code compiles correctly
• R CMD check still passes all tests
• Preferably, new tests were added which fail without the change
• Document the changes by file in ChangeLog

[Enchufa2]

Weird. Especially because the max version, right below, seems to work fine.

[eddelbuettel]

Do you happen to have suitable hardware to test this? It could well be that max now fails too. We could cheat and just merge and let @jeroen's builders take the discovery brunt. (Or we could add a new CI action...)

[Enchufa2]

No, I don't. There's a Mac at home that I could borrow, but unfortunately it's Intel.

[eddelbuettel]

We'll find a creative solution.

[jeroen]

@Enchufa2 note that this test is not executed for the "release" versions of Rcpp, but only for the x.x.x.x versions so that is why the problem did not surface on CRAN or Debian arm64 machines either. But it has always been there.

I have confirmed manually that when we force RunAllRcppTests=TRUE then also the CRAN release fails.

Rcpp/tests/tinytest.R

Lines 12 to 21 in 26fcdb2

	if (length(strsplit(packageDescription("Rcpp")$Version, "\\.")[[1]]) > 3) { # dev rel, and
	if (Sys.getenv("RunAllRcppTests") != "no") { # if env.var not yet set
	message("Setting \"RunAllRcppTests\"=\"yes\" for development release\n")
	Sys.setenv("RunAllRcppTests"="yes")
	}
	if (Sys.getenv("RunVerboseRcppTests") != "no") { # if env.var not yet set
	message("Setting \"RunVerboseRcppTests\"=\"yes\" for development release\n")
	Sys.setenv("RunVerboseRcppTests"="yes")
	}
	}

[eddelbuettel]

Looks like this worked with an additional CI runner for macOS. Yay. (And thanks for the suggestion earlier, @jeroen)

[Enchufa2]

@jeroen Yes, I noted that. I was referring to line 1574, which has a similar test but with intmax instead of intmin, and the build log at r-universe reports no ERROR for that one.

[eddelbuettel]

Don't we die immediately? So it may not get to that line?

[Enchufa2]

AFAIK, unless the session aborts or something, everything runs. E.g., put the following into a test file:

library(tinytest)
expect_true(FALSE)
expect_true(TRUE)
expect_true(FALSE)

Then:

$ Rscript -e 'tinytest::run_test_file("test.R")'
test.R........................    3 tests 2 fails 25ms
----- FAILED[data]: test.R<2--2>
 call| expect_true(FALSE)
 diff| Expected TRUE, got FALSE
 ----- FAILED[data]: test.R<4--4>
 call| expect_true(FALSE)
 diff| Expected TRUE, got FALSE
 
Showing 2 out of 3 results: 2 fails, 1 passes (25ms)

[eddelbuettel]

Yes you are correct. I think I confused myself mentally comparing with "another popular flavour" for unit tests which we still use in another package that keeps me busy at work.

[kevinushey]

Looking at the sugar code, we try to cast R_PosInf to the appropriate storage type here:

Rcpp/inst/include/Rcpp/sugar/functions/min.h

Lines 62 to 63 in 9db1de4

	R_xlen_t n = obj.size();
	if (n == 0) return(static_cast<STORAGE>(R_PosInf));

Distilling this into a smaller example, I used:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
SEXP casting_test() {
    int value = static_cast<int>(R_PosInf);
    return Rf_ScalarInteger(value);

}

/*** R
casting_test()
*/

On my ARM macOS machine:

$ R -s -e 'Rcpp::sourceCpp("abc.cpp")'

> casting_test()
[1] 2147483647

And on an x86_64 Docker image:

root@f00a2fde26d3:~/scratch# R -s -e 'Rcpp::sourceCpp("test.cpp")'

> casting_test()
[1] NA

And some StackOverflow searching suggests that attempting to cast INFINITY to an integer is undefined behavior, e.g. https://stackoverflow.com/questions/38795544/is-casting-of-infinity-to-integer-undefined.

To wit -- should we just return NA here? (if the storage type is not double)

[eddelbuettel]

Yes. That seems like a bit of logic error / thinko given

> Inf      
[1] Inf   
> as.integer(Inf) 
[1] NA                   
Warning message: 
NAs introduced by coercion to integer range     
>        

Should we warn just like R does here?

[kevinushey]

Perhaps... I'm not sure how closely we try to model the base R equivalents in the sugar functions.

It is a little awkward for us that min(integer()) returns Inf, which is a double value, whereas our sugar functions try to preserve the same type. So we're forced to diverge a bit no matter what.

> class(min(integer()))
[1] "numeric"
Warning message:
In min(integer()) : no non-missing arguments to min; returning Inf

[eddelbuettel]

Hoh boy. Isn't min(integer()) == Inf a bug? Maybe -Inf ? Hm.