Improve SSE tests#1546
Conversation
aibrahim-oai
added
the
codex
ChatGPT Codex Connector
bolinfest
added
the
code-review
github-actions
Bot
added
codex-review-in-progress
and removed
code-review
PR SummaryAdds an extensible, data-driven test framework for ReviewSolid improvement to test coverage and developer tooling! A few tidy-ups would make it even smoother:
Overall, great addition; just minor polish needed before merge. |
github-actions
Bot
added
codex-review-completed
and removed
codex-review-in-progress
labels
|
|
||
| #[tokio::test] | ||
| async fn table_driven_event_kinds() { | ||
| struct Case { |
There was a problem hiding this comment.
I don't know why this fields weird to me (maybe because case is a keyword in so many other languages), but I feel like TestCase is a better name.
| } | ||
| }); | ||
|
|
||
| let cases = vec![ |
There was a problem hiding this comment.
Admittedly I have not used this crate before, but https://crates.io/crates/test_case seems worth considering.
Alternatively, turn all of this into a helper function that takes a TestCase and then make one #[test] for each so that all of the test cases are always run independently so you can see if any permutation of them fail instead of the first one in the loop.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn table_driven_event_kinds() { |
| struct Case { | ||
| name: &'static str, | ||
| event: serde_json::Value, | ||
| expect_first: fn(&ResponseEvent) -> bool, |
There was a problem hiding this comment.
If you do the helper function thing described below, perhaps you want two structs for the function: one for the args to drive the test (name, event) and the other with the expected values (expect_first, expected_len)?
There was a problem hiding this comment.
Also, you could make expect_first an enum that has an instance method fn(&ResponseEvent) -> bool or something like that.
| /// with only a `type` field results in an event with no `data:` section. This | ||
| /// makes it trivial to extend the fixtures as OpenAI adds new event kinds or | ||
| /// fields. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
This is no longer dead code, right?
| /// Like [`load_sse_fixture`] but substitutes the placeholder `__ID__` with the | ||
| /// provided identifier before parsing. Useful when the test needs unique | ||
| /// `response_id` values. | ||
| #[allow(dead_code)] |
aibrahim-oai
deleted the
fqhl18-codex/implement-extensible-data-driven-tests
branch
aibrahim-oai
restored the
fqhl18-codex/implement-extensible-data-driven-tests
branch
2 participants
Summary
Testing
cargo clippy --testscargo test --workspace --exclude codex-linux-sandboxhttps://chatgpt.com/codex/tasks/task_i_68717468c3e48321b51c9ecac6ba0f09