refactor(protocols): extract responses tests to crates/protocols/tests/ - #1309
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 26 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR moves comprehensive serialization and deserialization tests from an inline Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase by moving the test suite for the responses protocol from crates/protocols/src/responses.rs to a dedicated file, crates/protocols/src/responses_tests.rs. The original file now references the new test file using the #[path] attribute. I have no feedback to provide.
817e020 to
a802498
Compare
slin1237
changed the title
|
Branch rewritten per feedback: tests relocated from |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/protocols/tests/responses.rs`:
- Around line 1-9: The tests file was moved to the integration-test location
(crates/protocols/tests/responses.rs) which changes test scope; to restore the
original unit-test visibility, move that file into
crates/protocols/src/responses_tests.rs and add a test-module wiring in
crates/protocols/src/responses.rs using the test-only module inclusion (i.e.,
add the cfg(test) path-based mod tests entry that references
responses_tests.rs), or if you intended integration tests instead, update the PR
description to state the change and confirm this new placement satisfies
visibility and discovery requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a30bf69-4e4b-48bf-9abb-1df9c9e67692
📒 Files selected for processing (2)
crates/protocols/src/responses.rscrates/protocols/tests/responses.rs
💤 Files with no reviewable changes (1)
- crates/protocols/src/responses.rs
Move the `#[cfg(test)] mod tests` block out of
`crates/protocols/src/responses.rs` into a standalone integration
test file `crates/protocols/tests/responses.rs`, matching the
convention already used by `background_mode_protocol.rs` and
`skills_protocol.rs`.
What changed
- `crates/protocols/src/responses.rs`: 3660 -> 2396 lines. Removed the
entire `#[cfg(test)] mod tests { ... }` block (previously L2398..EOF)
plus the single blank separator line above it.
- `crates/protocols/tests/responses.rs`: new 1262-line integration
test file. Contains all 41 tests from the removed inline module,
dedented one level. `use super::*;` was replaced with explicit
crate-qualified imports:
* `use openai_protocol::responses::*;` covers the majority of
types the tests reference (e.g., `ResponseOutputItem`,
`ResponseInputOutputItem`, `ResponseContentPart`, `ResponseTool`,
`ResponsesRequest`, `ResponsesToolChoice`, `McpTool`,
`Annotation`, `BuiltInToolChoiceType`,
`ImageGenerationCallStatus`, `IncludeField`,
`RequireApproval{,Mode,Rules,Filter}`,
`SimpleInputMessageTypeTag`, `SummaryTextContent`,
`FileDetail`, `StringOrContentParts`, the tool-choice tag
enums, `ResponsesFunctionToolChoice`, `ToolChoiceOptions`).
* `use openai_protocol::common::{ContextManagementType, Detail,
PromptCacheRetention, ToolChoice as ChatToolChoice,
ToolChoiceValue as ChatToolChoiceValue, ToolReference};`
resolves the aliases that the original inline test block
inherited via the private `use super::common::{...}` in
`responses.rs`. The `ChatToolChoice` / `ChatToolChoiceValue`
renames are preserved so the test bodies remain byte-for-byte
identical to the pre-move logic.
* The one fully-qualified path inside the tests
(`crate::common::ContextManagementType::Compaction`) was
shortened to `ContextManagementType::Compaction` now that the
type is imported.
Why
- The inline `mod tests` block had grown to 1264 lines (34% of
`responses.rs`). Relocating the tests keeps the production file
focused on protocol types.
- The protocols crate already uses `crates/protocols/tests/` for
protocol-surface integration tests (see `background_mode_protocol.rs`
and `skills_protocol.rs`). Placing the new file here matches that
convention rather than introducing a sibling source file, which the
user flagged as incorrect in PR review feedback on the earlier
attempt.
How
- Integration tests run as a separate binary against the public crate
API, so no test could rely on `pub(crate)` or `pub(super)` items.
Verified by checking every symbol the tests reference: all the types
used inside the test bodies are `pub` items exposed through
`openai_protocol::responses` and `openai_protocol::common`.
- Only the `use super::*;` line and the outer `mod tests { ... }`
braces were dropped; test logic is unchanged.
- No source code was edited. `responses.rs` keeps every `pub` item,
field, variant, and serde attribute untouched.
Verification
- `cargo test -p openai-protocol --test responses` -> 41 passed,
0 failed, 0 ignored. Matches the pre-move baseline of 41 tests.
- `cargo test -p openai-protocol` -> 53 (lib) + 9 + 41 + 18 + 1 doc
tests pass, 0 fail. The lib count dropped from 94 to 53 because the
41 relocated tests now run under the `responses` integration target.
- `cargo fmt --all -- --check` -> clean.
- `cargo clippy -p openai-protocol --tests -- -D warnings` -> clean.
No source-code behavior or public API was modified.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
a802498 to
0adb924
Compare
1 participant
Summary
Extracts the 1263-line
#[cfg(test)] mod testsblock fromcrates/protocols/src/responses.rsinto a standalone integration test file atcrates/protocols/tests/responses.rs, matching the existing convention used bybackground_mode_protocol.rsandskills_protocol.rs. Pure relocation; no test logic changed.History
The first attempt of this PR placed the tests in a sibling source file (
crates/protocols/src/responses_tests.rsvia#[path]). That was flagged as incorrect in review: protocol tests in this crate live undercrates/protocols/tests/as integration tests, not as sibling source files. This branch has been rewritten (force-pushed) to use the correct location.Stats
crates/protocols/src/responses.rs: 3660 -> 2396 lines (-1264).crates/protocols/tests/responses.rs: new, 1262 lines (1260 dedented test lines + 7 import lines + 1 blank + 1 module doc comment + trailing newline, minus the 2 droppeduse super::*/use serde_json::jsonlines).cargo test -p openai-protocoltotal pass count is preserved: 53 (lib) + 9 (interactions) + 41 (newtests/responses.rs) + 18 (existing integration) + 1 (doc) = 122, all passing. The 41 extracted tests match the pre-move baseline exactly.How
Integration tests run as a separate test binary against the public crate API, so they cannot use
pub(crate)orpub(super)items. Verified this was safe: every symbol referenced in the test bodies is apubitem exposed throughopenai_protocol::responsesoropenai_protocol::common.use super::*;was replaced with explicit crate-qualified imports at the top of the new file:The
ChatToolChoice/ChatToolChoiceValuerenames mirror the private aliases that were present inresponses.rs, so the test bodies remain byte-for-byte identical to the original inline logic. The single fully-qualified reference inside the tests (crate::common::ContextManagementType::Compaction) was shortened toContextManagementType::Compactionnow that the type is imported at the top of the file.Beyond those imports, the only mechanical transformation was:
#[cfg(test)] mod tests { ... }wrapper.use super::*;line.No
pubitem, field, variant, or serde attribute inresponses.rswas modified.Test plan
cargo test -p openai-protocol --test responses-> 41 passed, 0 failed, 0 ignored.cargo test -p openai-protocol-> all suites pass (lib 53, interactions 9, new responses 41, existing integration 18, doc 1).cargo fmt --all -- --check-> clean.cargo clippy -p openai-protocol --tests -- -D warnings-> clean.Checklist
cargo test -p openai-protocolpasses with the same test count (122)cargo test -p openai-protocol --test responsesruns the extracted 41 testscargo clippy -p openai-protocol --tests -- -D warningscleancargo fmt --all -- --checkcleanSummary by CodeRabbit