Skip to content

Conversation

@owenlin0
Copy link
Contributor

@owenlin0 owenlin0 commented Oct 27, 2025

There's a lot of visual noise in app-server's integration tests due to the number of .expect("<some_msg>") lines which are largely redundant / not very useful. Clean them up by using anyhow::Result + ? consistently.

Replaces the existing pattern of:

    let codex_home = TempDir::new().expect("create temp dir");
    create_config_toml(codex_home.path()).expect("write config.toml");

    let mut mcp = McpProcess::new(codex_home.path())
        .await
        .expect("spawn mcp process");
    timeout(DEFAULT_READ_TIMEOUT, mcp.initialize())
        .await
        .expect("initialize timeout")
        .expect("initialize request");

With:

    let codex_home = TempDir::new()?;
    create_config_toml(codex_home.path())?;

    let mut mcp = McpProcess::new(codex_home.path()).await?;
    timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;

@owenlin0 owenlin0 force-pushed the owen/clean_up_app_server_tests branch 2 times, most recently from 992a770 to 151be09 Compare October 27, 2025 23:58
@owenlin0 owenlin0 force-pushed the owen/clean_up_app_server_tests branch from 151be09 to d0bf5b2 Compare October 28, 2025 03:35
@owenlin0 owenlin0 marked this pull request as ready for review October 28, 2025 15:00
let _ok: SendUserMessageResponse = to_response::<SendUserMessageResponse>(response)
.expect("deserialize sendUserMessage response");
.await??;
let _ok: SendUserMessageResponse = to_response::<SendUserMessageResponse>(response)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

or is it a parsing test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea looks like it checks that the response parses. we do it a few times in this file

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 don't want to change the assertions in this pure refactor PR, so i'll leave this alone

@owenlin0 owenlin0 merged commit 2664192 into main Oct 28, 2025
34 of 36 checks passed
@owenlin0 owenlin0 deleted the owen/clean_up_app_server_tests branch October 28, 2025 15:10
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 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.

3 participants