Skip to content

make tests pass cleanly in sandbox#4067

Merged
nornagon-openai merged 8 commits intomainfrom
nornagon/fix-test
Sep 25, 2025
Merged

make tests pass cleanly in sandbox#4067
nornagon-openai merged 8 commits intomainfrom
nornagon/fix-test

Conversation

@nornagon-openai
Copy link
Copy Markdown
Collaborator

This changes the reqwest client used in tests to be sandbox-friendly, and skips a bunch of other tests that don't work inside the sandbox/without network.

Comment thread codex-rs/core/src/lib.rs Outdated
Comment thread codex-rs/core/tests/common/lib.rs
}

#[macro_export]
macro_rules! skip_if_sandbox {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

skip_if_seatbelt_sandbox?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

the intention here is to skip if there's any sandbox (since the sandboxes should behave roughly similarly irrespective of platform), but afaik we don't inject an env var for landlock?

Comment thread codex-rs/core/src/default_client.rs Outdated
.expect("user-agent header missing");
assert_eq!(ua_header.to_str().unwrap(), expected_ua);
#[test]
fn test_originator_header_defaults_to_codex_cli_rs() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This test was asserting that the value is being sent

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it was useful in it's old form

.unwrap_or_else(|_| reqwest::Client::new())
.default_headers(headers);
if is_sandboxed() {
builder = builder.no_proxy();
Copy link
Copy Markdown
Collaborator

@pakrym-oai pakrym-oai Sep 25, 2025

Choose a reason for hiding this comment

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

why was this part causing an issue? does it work in sandbox without proxy?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

can't look up the system proxy when in the sandbox.

Copy link
Copy Markdown
Collaborator

@pakrym-oai pakrym-oai left a comment

Choose a reason for hiding this comment

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

let's remove the duplicate macro
add a comment about why the proxy is being disabled
and bring back the originator header test

@nornagon-openai nornagon-openai merged commit 4a5f05c into main Sep 25, 2025
19 checks passed
@nornagon-openai nornagon-openai deleted the nornagon/fix-test branch September 25, 2025 20:11
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants