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

Use startup_file to force a standardized repos option during testing #650

Merged
merged 2 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 13 additions & 4 deletions crates/ark/src/fixtures/dummy_frontend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ struct DummyArkFrontendOptions {
r_environ: bool,
session_mode: SessionMode,
default_repos: DefaultRepos,
startup_file: Option<String>,
}

/// Wrapper around `DummyArkFrontend` that uses `SessionMode::Notebook`
Expand Down Expand Up @@ -110,7 +111,7 @@ impl DummyArkFrontend {
connection_file,
Some(registration_file),
r_args,
None,
options.startup_file,
options.session_mode,
false,
options.default_repos,
Expand Down Expand Up @@ -182,20 +183,27 @@ impl DerefMut for DummyArkFrontendNotebook {
impl DummyArkFrontendDefaultRepos {
/// Lock a frontend with a default repos setting.
///
/// NOTE: `startup_file` is required because you typically want
/// to force `options(repos =)` to a fixed value for testing, regardless
/// of what the caller's default `repos` are set as (i.e. rig typically
/// sets it to a non-`@CRAN@` value).
///
/// NOTE: Only one `DummyArkFrontend` variant should call `lock()` within
/// a given process.
pub fn lock(default_repos: DefaultRepos) -> Self {
Self::init(default_repos);
pub fn lock(default_repos: DefaultRepos, startup_file: String) -> Self {
Self::init(default_repos, startup_file);

Self {
inner: DummyArkFrontend::lock(),
}
}

/// Initialize with given default repos
fn init(default_repos: DefaultRepos) {
fn init(default_repos: DefaultRepos, startup_file: String) {
let mut options = DummyArkFrontendOptions::default();
options.default_repos = default_repos;
options.startup_file = Some(startup_file);

FRONTEND.get_or_init(|| Arc::new(Mutex::new(DummyArkFrontend::init(options))));
}
}
Expand Down Expand Up @@ -263,6 +271,7 @@ impl Default for DummyArkFrontendOptions {
r_environ: false,
session_mode: SessionMode::Console,
default_repos: DefaultRepos::Auto,
startup_file: None,
}
}
}
2 changes: 1 addition & 1 deletion crates/ark/src/repos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ fn apply_default_repos_auto() -> anyhow::Result<()> {
}

/// On Windows, we just use the RStudio CRAN mirror as the default.
#[cfg(windows)]
#[cfg(not(unix))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also tweaked this because we use #[cfg(unix)] above, so it seems cleanest to use not(unix) to ensure we dont miss anything

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense!

fn apply_default_repos_auto() -> anyhow::Result<()> {
apply_default_repos(DefaultRepos::RStudio)
}
Expand Down
14 changes: 13 additions & 1 deletion crates/ark/tests/repos-auto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,26 @@
//
//

use std::io::Write;

use amalthea::fixtures::dummy_frontend::ExecuteRequestOptions;
use ark::fixtures::DummyArkFrontendDefaultRepos;

/// Using the automatic repos setting, the default CRAN repo should be set to the global RStudio
/// CRAN mirror.
#[test]
fn test_auto_repos() {
let frontend = DummyArkFrontendDefaultRepos::lock(ark::repos::DefaultRepos::Auto);
// Use a startup file to force a standardized `repos` on startup,
// regardless of what your local R version has set (i.e. from rig)
let contents = r#"options(repos = c(CRAN = "@CRAN@"))"#;
let mut startup_file = tempfile::NamedTempFile::new().unwrap();
write!(startup_file, "{contents}").unwrap();
let startup_path = startup_file.path();

let frontend = DummyArkFrontendDefaultRepos::lock(
ark::repos::DefaultRepos::Auto,
startup_path.to_str().unwrap().to_string(),
);

let code = r#"getOption("repos")[["CRAN"]]"#;
frontend.send_execute_request(code, ExecuteRequestOptions::default());
Expand Down
37 changes: 31 additions & 6 deletions crates/ark/tests/repos-conf-file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,30 @@ use std::io::Write;
use amalthea::fixtures::dummy_frontend::ExecuteRequestOptions;
use ark::fixtures::DummyArkFrontendDefaultRepos;

/// Using a configuration file, set the default CRAN repo to a custom value.
/// Using a configuration file, set the default CRAN repo to a custom value,
/// and add an extra internal repo.
#[test]
fn test_conf_file_repos() {
let contents = r#"# Custom CRAN repo configuration file
CRAN=https://my.cran.mirror/
Internal=https://internal.cran.mirror/
"#;
let mut file = tempfile::NamedTempFile::new().unwrap();
write!(file, "{contents}").unwrap();
let mut conf_file = tempfile::NamedTempFile::new().unwrap();
write!(conf_file, "{contents}").unwrap();
let conf_path = conf_file.path();

let path = file.path();
let frontend =
DummyArkFrontendDefaultRepos::lock(ark::repos::DefaultRepos::ConfFile(path.to_path_buf()));
// Use a startup file to force a standardized `repos` on startup,
// regardless of what your local R version has set (i.e. from rig)
let contents = r#"options(repos = c(CRAN = "@CRAN@"))"#;
let mut startup_file = tempfile::NamedTempFile::new().unwrap();
write!(startup_file, "{contents}").unwrap();
let startup_path = startup_file.path();

let frontend = DummyArkFrontendDefaultRepos::lock(
ark::repos::DefaultRepos::ConfFile(conf_path.to_path_buf()),
startup_path.to_str().unwrap().to_string(),
);

let code = r#"getOption("repos")[["CRAN"]]"#;
frontend.send_execute_request(code, ExecuteRequestOptions::default());
Expand All @@ -37,5 +47,20 @@ Internal=https://internal.cran.mirror/

frontend.recv_iopub_idle();

assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count);

let code = r#"getOption("repos")[["Internal"]]"#;
frontend.send_execute_request(code, ExecuteRequestOptions::default());
frontend.recv_iopub_busy();

let input = frontend.recv_iopub_execute_input();
assert_eq!(input.code, code);
assert_eq!(
frontend.recv_iopub_execute_result(),
r#"[1] "https://internal.cran.mirror/""#
);
Comment on lines +50 to +61
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added testing for the Internal repo too to make sure it gets set


frontend.recv_iopub_idle();

assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count)
}
Loading