Skip to content

tests: serialize conversation item service tests#813

Closed
jbeckwith-oai wants to merge 3 commits into
mainfrom
codex/serialize-conversation-item-tests
Closed

tests: serialize conversation item service tests#813
jbeckwith-oai wants to merge 3 commits into
mainfrom
codex/serialize-conversation-item-tests

Conversation

@jbeckwith-oai

@jbeckwith-oai jbeckwith-oai commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • serialize the blocking and async conversation-item service tests within each JUnit worker
  • coordinate those tests across Gradle worker JVMs with an OS-backed file lock
  • keep the mock implementation and CI workflows unchanged

Root cause

Steady-generated conversation-item responses can be invalid for the SDK union discriminator/model validation when the blocking and async fixture tests access the shared mock server concurrently. JUnit resource locks cover concurrency inside one worker JVM; the file lock covers separate Gradle worker JVMs.

Validation

  • 20/20 cache-disabled focused repetitions with four Gradle workers
  • ./scripts/lint
  • ./scripts/test --no-build-cache --max-workers=4

Copilot AI review requested due to automatic review settings July 23, 2026 22:39
@jbeckwith-oai
jbeckwith-oai requested a review from a team as a code owner July 23, 2026 22:39
@openai-sdks

openai-sdks Bot commented Jul 23, 2026

Copy link
Copy Markdown

OkTest Summary

237/237 SDK tests passed in 14.631s for Java SDK PR #813.

Test results — 42 files
Test Result Time
tests/chat-completions-complex-body.test.ts ✅ Passed 448ms
tests/chat-completions-create.test.ts ✅ Passed 389ms
tests/chat-completions-stream.test.ts ✅ Passed 285ms
tests/files-content-binary.test.ts ✅ Passed 293ms
tests/files-create-multipart.test.ts ✅ Passed 284ms
tests/files-list-pagination.test.ts ✅ Passed 257ms
tests/initialize-config.test.ts ✅ Passed 183ms
tests/instance-isolation.test.ts ✅ Passed 111ms
tests/models-list.test.ts ✅ Passed 235ms
tests/responses-background-lifecycle.test.ts ✅ Passed 282ms
tests/responses-body-method-errors.test.ts ✅ Passed 495ms
tests/responses-cancel-timeout.test.ts ✅ Passed 230ms
tests/responses-cancel.test.ts ✅ Passed 264ms
tests/responses-compact-retries.test.ts ✅ Passed 327ms
tests/responses-compact.test.ts ✅ Passed 254ms
tests/responses-create-advanced-stream.test.ts ✅ Passed 523ms
tests/responses-create-advanced.test.ts ✅ Passed 1.242s
tests/responses-create-disconnect.test.ts ✅ Passed 1.098s
tests/responses-create-errors.test.ts ✅ Passed 339ms
tests/responses-create-malformed-api-responses.test.ts ✅ Passed 425ms
tests/responses-create-retries.test.ts ✅ Passed 250ms
tests/responses-create-stream-failures.test.ts ✅ Passed 176ms
tests/responses-create-stream-timeout.test.ts ✅ Passed 212ms
tests/responses-create-stream-wire.test.ts ✅ Passed 5.335s
tests/responses-create-stream.test.ts ✅ Passed 118ms
tests/responses-create-terminal-states.test.ts ✅ Passed 336ms
tests/responses-create-timeout.test.ts ✅ Passed 266ms
tests/responses-create.test.ts ✅ Passed 541ms
tests/responses-delete.test.ts ✅ Passed 250ms
tests/responses-input-items-errors.test.ts ✅ Passed 286ms
tests/responses-input-items-list.test.ts ✅ Passed 290ms
tests/responses-input-items-options.test.ts ✅ Passed 163ms
tests/responses-input-tokens-count-timeout.test.ts ✅ Passed 193ms
tests/responses-input-tokens-count.test.ts ✅ Passed 293ms
tests/responses-malformed-inputs.test.ts ✅ Passed 4.308s
tests/responses-not-found-errors.test.ts ✅ Passed 349ms
tests/responses-parse.test.ts ✅ Passed 510ms
tests/responses-retrieve-retries.test.ts ✅ Passed 328ms
tests/responses-retrieve.test.ts ✅ Passed 217ms
tests/responses-stored-method-errors.test.ts ✅ Passed 918ms
tests/retry-behavior.test.ts ✅ Passed 3.556s
tests/sdk-error-shape.test.ts ✅ Passed 430ms

View OkTest run #30059918772

SDK merge (35dfcbc1a231) · head (47070db59e34) · base (a3a50ec79890) · OkTest (d2b5921fc22a)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Serializes the blocking and async conversation item service fixture tests to prevent flaky deserialization/model-validation failures caused by concurrent execution under JUnit 5 parallel test mode.

Changes:

  • Adds a shared JUnit @ResourceLock to the blocking conversation item service tests.
  • Adds the same shared JUnit @ResourceLock to the async conversation item service tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
openai-java-core/src/test/kotlin/com/openai/services/blocking/conversations/ItemServiceTest.kt Adds a class-level @ResourceLock to serialize conversation item response deserialization tests.
openai-java-core/src/test/kotlin/com/openai/services/async/conversations/ItemServiceAsyncTest.kt Applies the same class-level @ResourceLock so blocking + async tests don’t run concurrently.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c7551b9b8e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@jbeckwith-oai
jbeckwith-oai force-pushed the codex/serialize-conversation-item-tests branch from c7551b9 to 67b59c4 Compare July 23, 2026 23:06

@HAYDEN-OAI HAYDEN-OAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One actionable correctness issue: the proposed class-level JUnit lock does not synchronize the separate Gradle worker JVMs that share the mock-server address.

Copilot AI review requested due to automatic review settings July 23, 2026 23:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 24, 2026 01:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@HAYDEN-OAI HAYDEN-OAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The prior cross-worker serialization finding is resolved: the focused OS-backed lock now coordinates the affected classes across Gradle worker JVMs, while the existing resource lock covers in-JVM concurrency. No further actionable findings.

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #815. That merged change generalizes the same process-local and OS-backed serialization to every test using the shared Steady mock server, including the conversation-item tests covered here, so merging this narrower lock would be redundant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants