Add tests for OpenAI helpers and retry logic by aibrahim-oai · Pull Request #1547 · openai/codex

aibrahim-oai · 2025-07-11T21:37:41Z

Summary

add unit tests for tool JSON helpers
verify message assembly for chat completions
test retry and error handling paths of ModelClient

Testing

cargo clippy --workspace --all-targets -- -D warnings
cargo test --workspace --exclude codex-linux-sandbox

https://chatgpt.com/codex/tasks/task_i_68717e8603a48321b875080ed3b70d63

github-actions · 2025-07-12T18:57:51Z

PR Summary

Adds a suite of unit tests (≈ 437 LoC) for Rust core:

verifies JSON tool payload construction (openai_tools.rs)
checks chat-completion message assembly (chat_completions.rs)
exercises retry / back-off logic and error handling in the model client (client.rs)

Review

Nice boost to test coverage and confidence in critical request paths!

✅ Tests are well-scoped, use wiremock to avoid real calls, and guard against the sandbox flag.
🔄 Consider resetting the env vars (OPENAI_REQUEST_MAX_RETRIES) after each test to avoid bleed-through.
🔄 pretty_assertions appears in the code—ensure it is listed as a dev-dependency in Cargo.toml if not already.
📝 Minor: a few unwrap()s could become expect() with context, but fine for tests.

Overall, looks solid—just the small cleanup above and good to merge.

View workflow run

bolinfest

I have a lot of comments, but I think none of these is review blocking, so please address before submitting.

bolinfest · 2025-07-12T18:44:53Z

+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn assembles_messages_correctly() {
+        let server = MockServer::start().await;


Does this also need to check CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR?

bolinfest · 2025-07-12T18:46:04Z

+
+        let body = capture.lock().unwrap().take().unwrap();
+        let messages = body.get("messages").unwrap().as_array().unwrap();
+        assert_eq!(messages[1]["role"], "user");


Can we just do one assert_eq! on messages in its entirety? Or maybe &messages[1..]?

bolinfest · 2025-07-12T18:49:48Z

+        }
+    }
+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]


Please add a docstring explaining what is being tested.

bolinfest · 2025-07-12T18:51:04Z

    tokio::spawn(process_sse(stream, tx_event));
    Ok(ResponseStream { rx_event })
 }
+#[cfg(test)]


Looks like you need just fmt.

bolinfest · 2025-07-12T18:52:45Z

+        .unwrap();
+        cfg.model_provider = provider.clone();
+        cfg.model = "gpt-test".into();
+        Arc::new(cfg)


Just FYI, codex_home will be deleted when this function exits, but that seems fine in this case.

bolinfest · 2025-07-12T18:55:10Z

+    }
+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn retries_once_on_server_error() {


I think all of these tests would benefit from docstrings.

bolinfest · 2025-07-12T19:02:34Z

+        let provider = ModelProviderInfo {
+            name: "openai".into(),
+            base_url: format!("{}/v1", server.uri()),
+            env_key: Some("PATH".into()),
+            env_key_instructions: None,
+            wire_api: WireApi::Responses,
+            query_params: None,
+            http_headers: None,
+            env_http_headers: None,
+        };
+        let config = default_config(provider.clone());
+        let client = ModelClient::new(
+            config,
+            provider,
+            ReasoningEffortConfig::None,
+            ReasoningSummaryConfig::None,
+        );


Maybe use a helper function to dedupe common logic in tests?

bolinfest · 2025-07-12T19:03:16Z

+
+        let tools = create_tools_json_for_responses_api(&prompt, "gpt-4").unwrap();
+        assert_eq!(tools.len(), 2);
+        assert_eq!(tools[0]["type"], "function");


Just one assert_eq! for all of tools[0]?

bolinfest · 2025-07-12T19:05:25Z

+        assert!(
+            tools
+                .iter()
+                .any(|t| t.get("name") == Some(&name.clone().into()))


Maybe use find(|t| t.get("name").as_ref() == Some("srv.dummy") on tools.iter() or something like that and then do an assert_eq!() on the value returned from find()?

bolinfest · 2025-07-12T19:07:30Z

+        );
+    }
+
+    #[test]


For both of these tests, can we just assert the entire string/serde_json::Value that we get back? I realize this means that we will have to update this test if we change the default tools, but I think having a test that verifies everything (and effectively documents what we send on the wire) is worth that maintenance cost.

…b/actions/codex (#1507) [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=@types/node&package-manager=bun&previous-version=22.15.21&new-version=24.0.12)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore(deps-dev): bump @types/bun from 1.2.13 to 1.2.18 in /.github/actions/codex (#1509) [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=@types/bun&package-manager=bun&previous-version=1.2.13&new-version=1.2.18)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Add paste summarization to Codex TUI (#1549) - introduce `Paste` event to avoid per-character paste handling - collapse large pasted blocks to `[Pasted Content X lines]` - store the real text so submission still includes it - wire paste handling through `App`, `ChatWidget`, `BottomPane`, and `ChatComposer` - `cargo test -p codex-tui` ------ https://chatgpt.com/codex/tasks/task_i_6871e24abf80832184d1f3ca0c61a5ee https://github.com/user-attachments/assets/eda7412f-da30-4474-9f7c-96b49d48fbf8 addressing review addressing review addressing review Fix clippy docstring

…ruction

This reverts commit 75a1e4b.

…ruction

Add unit tests for OpenAI request helpers and retry logic

bdc60ef

aibrahim-oai added the codex Label used by connector to tag PRs that have been reviewed by Codex label Jul 11, 2025 — with ChatGPT Codex Connector

aibrahim-oai requested review from bolinfest and gpeal July 11, 2025 22:25

bolinfest added the code-review Issues relating to code reviews performed by codex label Jul 12, 2025

github-actions Bot added codex-review-in-progress and removed code-review Issues relating to code reviews performed by codex labels Jul 12, 2025

github-actions Bot added codex-review-completed and removed codex-review-in-progress labels Jul 12, 2025

bolinfest approved these changes Jul 12, 2025

View reviewed changes

dependabot Bot and others added 2 commits July 12, 2025 16:38

docstring

3d85eab

aibrahim-oai force-pushed the codex/implement-tests-for-json-payload-construction branch from 61f28bc to 3d85eab Compare July 12, 2025 23:40

aibrahim-oai added 13 commits July 12, 2025 17:26

Merge branch 'main' into codex/implement-tests-for-json-payload-const…

7d316c9

…ruction

Merge branch 'main' into codex/implement-tests-for-json-payload-const…

650e08f

…ruction

fmt

39f88bc

cargo test

aeb12fc

fixing tests

86be2a6

Merge branch 'main' into codex/implement-tests-for-json-payload-const…

e4f6b76

…ruction

adding helper to prevent caching

75a1e4b

Revert "adding helper to prevent caching"

3330466

This reverts commit 75a1e4b.

convert to config

86dcd0b

Merge branch 'main' into codex/implement-tests-for-json-payload-const…

b66180a

…ruction

adressing reviews

514b3cd

adressing reviews

b4b255e

adressing reviews

874a6a9

aibrahim-oai requested a review from bolinfest July 14, 2025 22:39

aibrahim-oai marked this pull request as draft July 16, 2025 17:02

aibrahim-oai closed this Jul 17, 2025

github-actions Bot locked and limited conversation to collaborators Jul 17, 2025

+                      );
+                  }
+                  #[test]

Conversation

aibrahim-oai commented Jul 11, 2025

Summary

Testing

Uh oh!

github-actions Bot commented Jul 12, 2025

PR Summary

Review

Uh oh!

bolinfest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants