Add testing on Windows, macOS, and R 4.2 #545
Conversation
|
There seems to be some kind of issue occasionally. I sometimes see this locally and one of our builds above shows it too |
|
hmm that's surprising these dummy frontend sockets are on ark/crates/amalthea/src/fixtures/dummy_frontend.rs Lines 88 to 95 in 77d9400 Since you can see it locally, maybe you could set a breakpoint and explore with e.g. |
DavisVaughan
force-pushed
the
feature/more-testing
branch
3 times, most recently
from
5a6db98 to
d0a4f7b
Compare
DavisVaughan
changed the title
| bin_edges <- as.POSIXct(h$breaks, tz = attr(x, "tzone")) | ||
| # Must supply an `origin` on R <= 4.2 | ||
| origin <- as.POSIXct("1970-01-01", tz = "UTC") | ||
| bin_edges <- as.POSIXct(h$breaks, tz = attr(x, "tzone"), origin = origin) |
There was a problem hiding this comment.
Bug found when adding 4.2 CI
| # Must open in binary write mode, otherwise even though we set | ||
| # `eol = "\n"` on Windows it will still write `\r\n` due to the C | ||
| # level `vfprintf()` call. | ||
| con <- file(path, open = "wb") | ||
| defer(close(con)) |
There was a problem hiding this comment.
Bug found when adding Windows CI. Tests expect \n as the line ending
And the way we do readChar(path, file.info(path)$size - 1L, useBytes = TRUE) above expects 1 char for the trailing line (i.e. \n not \r\n)
crates/ark/src/sys/windows/locale.rs
Outdated
| fn test_locale() { | ||
| // These tests assert that we've embedded our Application Manifest file correctly in `build.rs` | ||
| r_test(|| { | ||
| let latin1 = harp::parse_eval_base("l10n_info()$`Latin-1`").unwrap(); | ||
| let latin1 = bool::try_from(latin1).unwrap(); | ||
| assert!(!latin1); | ||
|
|
||
| let utf8 = harp::parse_eval_base("l10n_info()$`UTF-8`").unwrap(); | ||
| let utf8 = bool::try_from(utf8).unwrap(); | ||
| assert!(utf8); | ||
|
|
||
| let codepage = harp::parse_eval_base("l10n_info()$codepage").unwrap(); | ||
| let codepage = i32::try_from(codepage).unwrap(); | ||
| assert_eq!(codepage, 65001); | ||
|
|
||
| let system_codepage = harp::parse_eval_base("l10n_info()$system.codepage").unwrap(); | ||
| let system_codepage = i32::try_from(system_codepage).unwrap(); | ||
| assert_eq!(system_codepage, 65001); |
There was a problem hiding this comment.
Tests on both the ark and harp side that the application manifest was correctly embedded into the test binary
| #' Write out templated Windows Application Manifest files | ||
| #' | ||
| #' We use two files to embed an application manifest on Windows: | ||
| #' - `{crate}-manifest.rc` | ||
| #' - `{crate}.exe.manifest` | ||
| #' | ||
| #' And we generate these two files for both `harp` and `ark`. | ||
| #' | ||
| #' They are effectively the same, we just need to swap out the crate names, | ||
| #' so we use this script to write the files in a consistent manner. | ||
| write_manifest <- function(root, crate) { |
There was a problem hiding this comment.
Because the manifest files are almost identical between ark and harp, we generate them from templates
| - uses: r-lib/actions/setup-r@v2 | ||
| with: | ||
| r-version: ${{ matrix.config.r }} | ||
| use-public-rspm: true |
There was a problem hiding this comment.
Does this mean we can install binary packages from p3m?
By the way there's a bunch of tests skipped because data.table and tibble are not installed on CI. We could now easily fix that right?
E.g. using https://github.com/r-lib/actions/tree/v2/setup-r-dependencies with extra-packages or something (hopefully it still works without a DESCPRIPTION file but we could add one if needed).
And also do this for the harp test binary, which starts R and tests UTF-8 related capabilities.
DavisVaughan
force-pushed
the
feature/more-testing
branch
from
035ffcd to
767d495
Compare
This PR has the goal of getting us the following CI structure:
Adding support for macOS was incredibly simple.
Adding support for R 4.2 revealed one bug that I've fixed and called out below.
Adding support for Windows has been a nightmare. Further details below.
You should be able to trigger each OS's workflow manually through workflow dispatch, and you should also be able to turn on SSH-ing into the Linux workflow (it will pause right before running
cargo test).Application manifest
I noticed that during tests on Windows,
sessionInfo()reports that we are not in a UTF-8 locale and we are on Windows Server 2012, even though the github machine is Windows Server 2022. This is a classic sign that our application manifest file wasn't being used #178.The manifest is being used for our main binary,
ark.exe, but it wasn't being embedded into our test binary, likeark-<stuff>.exe. Making that happen was a little complicated. The crux of it is:embed_resource::compile()setscargo:rustc-link-arg-bins, which doesn't target test binariesembed_resource::compile_for_tests()setscargo:rustc-link-arg-tests, but that doesn't target unit test binaries due to a Rust bugembed_resource::compile_for_everything()setscargo:rustc-link-ark, which is the "just do it for everything" approach, and that does work. This didn't exist though, I had to ask the embed-resource maintainer for it!So that allowed ark's unit tests to start up R in a way that R was in a UTF-8 locale and ran under Windows Server 2022.
But harp also starts R for its unit tests! And it also tests UTF-8 behavior! So I had to add a
build.rsscript to harp to also embed the manifest file there as well.Open comms timing issues
See #548, I seemed to hit this error every now and then on the Windows CI and my local Windows VM
Outstanding issues
Occasionally the test suite will hang on windows after running
test_kernel, but I cannot figure out why. I get no information except eventually it says something like:My guess is that a zmq
send()orrecv()is stuck, so I've setset_sndtimeo()andset_rectimeo()to try and turn these cases into a panic at test time, but I actually haven't been able to reproduce the issue since I added them, and can't reproduce this one locally.