Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add testing on Windows, macOS, and R 4.2 #545

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
dcb8834
Explore testing on windows
DavisVaughan Sep 25, 2024
2f9a43b
Don't need `r-base-dev`, right?
DavisVaughan Sep 25, 2024
a2f3da2
Fix typo
DavisVaughan Sep 25, 2024
0241dde
I think `cargo build` before `cargo test` is redundant?
DavisVaughan Sep 25, 2024
1f1488a
Write in binary mode to ensure `eol` is respected on Windows
DavisVaughan Sep 25, 2024
a3f3293
Ensure file is closed before getting file size
DavisVaughan Sep 25, 2024
23dc802
Can it even roundtrip correctly?
DavisVaughan Sep 25, 2024
f2af98e
Did it get into R right?
DavisVaughan Sep 25, 2024
b3f8269
See if `&str` to `RObject` loses encoding
DavisVaughan Sep 25, 2024
62d61d5
What is the encoding?
DavisVaughan Sep 25, 2024
e1d28e4
What about the windows version?
DavisVaughan Sep 25, 2024
86a3611
make sure build.rs is actually being called
DavisVaughan Sep 25, 2024
c8b3734
panic after to get stderr output
DavisVaughan Sep 25, 2024
09c02c8
Remove panic
DavisVaughan Sep 25, 2024
e827c7f
does this happen to work?
DavisVaughan Sep 25, 2024
6ada120
what does an integration test report?
DavisVaughan Sep 25, 2024
edbae6c
Use `compile_for_everything()` to embed the manifest in the test binary
DavisVaughan Sep 26, 2024
a8c175e
Try removing some extra installs
DavisVaughan Sep 26, 2024
cd81142
Create unified `test.yml`
DavisVaughan Sep 26, 2024
e98cff9
Make the subworkflows callable
DavisVaughan Sep 26, 2024
d0a4f7b
Add script to auto generate the manifest files
DavisVaughan Sep 27, 2024
899e2c7
Post rebase fixup
DavisVaughan Sep 27, 2024
17c5b84
Only test on nightly on 1 OS, also test on R 4.2
DavisVaughan Sep 27, 2024
6024d60
Less repetition
DavisVaughan Sep 27, 2024
9bc08e2
Add in macOS testing
DavisVaughan Sep 27, 2024
a6581e9
An `origin` is required on R <= 4.2 for `as.POSIXct.numeric()`
DavisVaughan Sep 27, 2024
fbb3354
Looks better without this too
DavisVaughan Sep 27, 2024
51a899f
Allow SSH access on workflow dispatch
DavisVaughan Sep 27, 2024
0b0aab9
Try and get some information on where `recv()` is hanging
DavisVaughan Sep 27, 2024
36357c4
Send timeout too
DavisVaughan Sep 27, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 0 additions & 45 deletions .github/workflows/amalthea-ci.yml

This file was deleted.

52 changes: 52 additions & 0 deletions .github/workflows/test-linux.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
name: "Test Ark - Linux"

on:
workflow_call:
workflow_dispatch:
inputs:
ssh:
description: 'Set up an SSH session before running `cargo test`?'
type: boolean
required: true
default: false

jobs:
linux:
runs-on: ubuntu-latest
name: "Rust: ${{ matrix.config.rust }}, R: ${{ matrix.config.r }}"
strategy:
fail-fast: false
matrix:
config:
- { rust: 'stable', r: 'release' }
# Oldest supported R version
- { rust: 'stable', r: '4.2' }
# Nightly rust
- { rust: 'nightly', r: 'release' }
timeout-minutes: 30
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
steps:
- uses: actions/checkout@v4

- uses: dtolnay/rust-toolchain@nightly
if: matrix.config.rust == 'nightly'

- uses: r-lib/actions/setup-r@v2
with:
r-version: ${{ matrix.config.r }}
use-public-rspm: true

- name: Setup Build Environment
run: |
sudo apt-get update

- name: Setup SSH access
uses: mxschmitt/action-tmate@v3
if: ${{ inputs.ssh }}
timeout-minutes: 30

- name: Run Unit Tests
id: test
run: |
cargo test --verbose
30 changes: 30 additions & 0 deletions .github/workflows/test-macos.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
name: "Test Ark - macOS"

on:
workflow_call:
workflow_dispatch:

jobs:
macos:
runs-on: macos-latest
name: "Rust: ${{ matrix.config.rust }}, R: ${{ matrix.config.r }}"
strategy:
fail-fast: false
matrix:
config:
- { rust: 'stable', r: 'release' }
timeout-minutes: 30
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
steps:
- uses: actions/checkout@v4

- uses: r-lib/actions/setup-r@v2
with:
r-version: ${{ matrix.config.r }}
use-public-rspm: true

- name: Run Unit Tests
id: test
run: |
cargo test --verbose
30 changes: 30 additions & 0 deletions .github/workflows/test-windows.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
name: "Test Ark - Windows"

on:
workflow_call:
workflow_dispatch:

jobs:
windows:
runs-on: windows-latest
name: "Rust: ${{ matrix.config.rust }}, R: ${{ matrix.config.r }}"
strategy:
fail-fast: false
matrix:
config:
- { rust: 'stable', r: 'release' }
timeout-minutes: 30
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
steps:
- uses: actions/checkout@v4

- uses: r-lib/actions/setup-r@v2
with:
r-version: ${{ matrix.config.r }}
use-public-rspm: true

- name: Run Unit Tests
id: test
run: |
cargo test --verbose
23 changes: 23 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
name: "Test Ark"
on:
push:
branches:
- main
pull_request:
workflow_dispatch:

jobs:
test_macos:
name: Test macOS
uses: ./.github/workflows/test-macos.yml
secrets: inherit

test_windows:
name: Test Windows
uses: ./.github/workflows/test-windows.yml
secrets: inherit

test_linux:
name: Test Linux
uses: ./.github/workflows/test-linux.yml
secrets: inherit
14 changes: 8 additions & 6 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions crates/amalthea/src/socket/socket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,15 @@ impl Socket {
}
}

// 10 second timeout on `recv()` operations (for testing for now)
// TODO: Expose `IS_TESTING` in its own lightweight crate?
if let Err(error) = socket.set_rcvtimeo(10000) {
return Err(Error::CreateSocketFailed(name, error));
}
if let Err(error) = socket.set_sndtimeo(10000) {
return Err(Error::CreateSocketFailed(name, error));
}
Comment on lines +136 to +143
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Remove or hide behind test flag

The goal is to avoid an infinite hang on send/recv issues during kernel tests


// Set the socket's identity, if supplied
if let Some(identity) = identity {
if let Err(err) = socket.set_identity(identity) {
Expand Down
2 changes: 1 addition & 1 deletion crates/ark/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ harp = { path = "../harp", features = ["testing"]}

[build-dependencies]
chrono = "0.4.23"
embed-resource = "2.4.0"
embed-resource = "2.5.0"

[package.metadata.generate-rpm]
assets = [{ source = "target/release/ark", dest = "/usr/bin/ark", mode = "755" }]
Expand Down
17 changes: 16 additions & 1 deletion crates/ark/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,23 @@ fn main() {

// Embed an Application Manifest file on Windows.
// Documented to do nothing on non-Windows.
// We also do this for harp to support its unit tests.
//
// We can't just use `compile()`, as that uses `cargo:rustc-link-arg-bins`,
// which targets the main `ark.exe` (good) but not the test binaries (bad).
// We need the application manifest to get embedded into the ark/harp test
// binaries too, so that the instance of R started by our tests also has
// UTF-8 support.
//
// We can't use `compile_for_tests()` because `cargo:rustc-link-arg-tests`
// only targets integration tests right now, not unit tests.
// https://github.com/rust-lang/cargo/issues/10937
//
// Instead we use `compile_for_everything()` which uses the kitchen sink
// instruction of `cargo:rustc-link-arg`, and that seems to work.
// https://github.com/nabijaczleweli/rust-embed-resource/issues/69
let resource = Path::new("resources")
.join("manifest")
.join("ark-manifest.rc");
embed_resource::compile(resource, embed_resource::NONE);
embed_resource::compile_for_everything(resource, embed_resource::NONE);
}
4 changes: 3 additions & 1 deletion crates/ark/resources/manifest/ark-manifest.rc
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// Autogenerated, do not modify! See `template.rc`.

// This is a C file that embed-resource compiles for us.
// It helps embed our Windows Application Manifest file.
// https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests
Expand All @@ -7,5 +9,5 @@
// https://github.com/nabijaczleweli/rust-embed-resource/issues/11#issuecomment-779295722
#include <winuser.h>

// The `1` is a `resource_id` that specifies ark as an executable file
// The `1` is a `resource_id` that specifies us as an executable file
1 RT_MANIFEST "ark.exe.manifest"
1 change: 1 addition & 0 deletions crates/ark/resources/manifest/ark.exe.manifest
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

<assemblyIdentity version="1.0.0.0" name="ark" type="win32"/>

<!-- Autogenerated, do not modify! See `template.manifest`. -->
<description>ark</description>

<!-- Support UTF-8 code page with R 4.2.0 and newer. -->
Expand Down
41 changes: 34 additions & 7 deletions crates/ark/src/modules/positron/r_data_explorer.R
Original file line number Diff line number Diff line change
Expand Up @@ -327,13 +327,38 @@ export_selection <- function(x, format = c("csv", "tsv", "html"), include_header
}

write_delim <- function(x, delim, include_header) {
tmp <- tempfile()
defer(unlink(tmp))
path <- tempfile()
defer(unlink(path))

utils::write.table(x, tmp, sep = delim, row.names = FALSE, col.names = include_header, quote = FALSE, na = "")
# We use size - 1 because we don't want to read the last newline character
# that creates problems when pasting the content in spreadsheets
readChar(tmp, file.info(tmp)$size - 1L)
write_delim_impl(x, path, delim, include_header)

# We use `size - 1` because we don't want to read the last newline character
# as that creates problems when pasting the content in spreadsheets.
# `file.info()$size` reports the size in bytes, hence `useBytes = TRUE`.
readChar(path, file.info(path)$size - 1L, useBytes = TRUE)
}

write_delim_impl <- function(x, path, delim, include_header) {
# Scope the `con` lifetime to just this helper.
# We need to `close()` the connection before we try and get the
# `file.info()$size`.

# 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))
Comment on lines +346 to +350
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


utils::write.table(
x = x,
file = con,
sep = delim,
eol = "\n",
row.names = FALSE,
col.names = include_header,
quote = FALSE,
na = ""
)
}

write_html <- function(x, include_header) {
Expand Down Expand Up @@ -400,7 +425,9 @@ profile_histogram <- function(x, method = c("fixed", "sturges", "fd", "scott"),

# For dates, we convert back the breaks to the date representation.
if (inherits(x, "POSIXct")) {
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)
Comment on lines -403 to +430
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug found when adding 4.2 CI

}

list(
Expand Down
1 change: 1 addition & 0 deletions crates/ark/src/sys/windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
pub mod console;
pub mod control;
pub mod interface;
mod locale;
pub mod path;
pub mod signals;
mod strings;
Expand Down
Loading
Loading