Skip to content

test: add 12 integration tests covering identified coverage gaps#2845

Merged
markphelps merged 6 commits intomainfrom
integration-test-coverage-gaps
Mar 17, 2026
Merged

test: add 12 integration tests covering identified coverage gaps#2845
markphelps merged 6 commits intomainfrom
integration-test-coverage-gaps

Conversation

@markphelps
Copy link
Contributor

Summary

  • Adds 12 new integration tests covering gaps identified by auditing the existing 125-test suite
  • Enhances the test harness to support error body assertions and webhook error message capture

Harness Enhancements

  • ! curl now writes the HTTP response body to stderr, enabling tests to assert on error response content (e.g., 409 bodies)
  • webhookResult captures the actual error_message string (not just has_error bool), enabling assertion on failed webhook payloads

New Tests

Test Gap Covered
prediction_error_response JSON response shape when predict() raises ValueError/RuntimeError/Exception
nested_output_types BaseModel-in-BaseModel, List[BaseModel], Optional nested fields
iterator_error_midstream Generator yields partial output then raises — webhook reports failure
predict_sys_exit sys.exit() fails prediction but worker survives for next request
TestConcurrentAboveLimit max_concurrency + 1 returns 409 Conflict with capacity error
sequential_state_leak Documents module-level state persistence across sequential predictions
webhook_delivery_failure Server stays healthy when webhook URL is unreachable
TestSIGTERMDuringSetup SIGTERM during slow setup() causes clean shutdown
webhook_prediction_error Failed prediction webhook payload includes error message
build_openapi_schema_complex Schema for constraints, choices, Secret, Optional, structured output
coglet_large_file_upload_serial 50 MiB binary file through upload URL pipeline
async_generator_precollect Async generator output is pre-collected into response array

Test Types

  • 10 txtar tests in integration-tests/tests/ (declarative testscript format)
  • 2 Go-native tests in integration-tests/concurrent/ (require parallel HTTP coordination)

How to Run

mise run build:cog && mise run build:sdk && mise run build:coglet:wheel:linux-x64
COG_BINARY=dist/go/*/cog mise run test:integration

…rtions

- ! curl now writes response body to stderr for error body assertions
- webhookResult captures ErrorMessage string for failed prediction webhook tests
…or errors, sys.exit

- prediction_error_response: verify JSON shape for ValueError/RuntimeError/Exception
- nested_output_types: BaseModel-in-BaseModel, List[BaseModel], Optional nested fields
- iterator_error_midstream: generator yields then raises, webhook captures failure
- predict_sys_exit: sys.exit() fails prediction but worker survives for next request
…k failure, SIGTERM

- TestConcurrentAboveLimit: 409 when exceeding max_concurrency
- TestSIGTERMDuringSetup: clean shutdown when SIGTERM arrives during setup
- sequential_state_leak: documents module-level state persistence across calls
- webhook_delivery_failure: server stays healthy when webhook URL is unreachable
- webhook_prediction_error: failed prediction webhook payload has error fields
…ync generator

- build_openapi_schema_complex: constraints, choices, Secret, Optional, BaseModel output
- coglet_large_file_upload_serial: 50 MiB binary file through upload URL pipeline
- async_generator_precollect: verify async generator output is pre-collected
@markphelps markphelps requested a review from a team as a code owner March 16, 2026 20:21
@markphelps markphelps marked this pull request as draft March 16, 2026 20:28
…dstream

nested_output_types: rewrite test to use supported types only. Cog's type
system does not support nested BaseModel fields (FieldType.from_type raises
ValueError for custom types without a Coder). Test now covers multiple
primitive types, Optional, and List[str] in a flat BaseModel.

iterator_error_midstream: fix webhook harness Output field type. The webhook
payload's output field is a JSON array for iterator predictions (not a string),
causing json.Decode to fail with UnmarshalTypeError. The harness returned 400,
coglet gave up (400 is not retryable), and the test hung forever. Changed
Output from string to json.RawMessage to handle both string and array outputs.
@markphelps markphelps marked this pull request as ready for review March 16, 2026 20:52
@markphelps markphelps requested a review from michaeldwan March 16, 2026 20:54
@markphelps markphelps enabled auto-merge (squash) March 17, 2026 16:38
- Fix TestSIGTERMDuringSetup sending SIGINT instead of SIGTERM (use syscall.SIGTERM)
- Replace 500ms sleep race in TestConcurrentAboveLimit with polling retry for 409
- Replace 3s sleep race in TestSIGTERMDuringSetup with health-check polling for STARTING status
- Strengthen predict_time assertion in async_generator_precollect to verify numeric value
- Add comment explaining regex dot-as-quote workaround in sequential_state_leak
@markphelps
Copy link
Contributor Author

Code Review Fixes

Pushed 1f1531ce addressing the review feedback. Here's what changed and what I intentionally left as-is:

Fixed

# Issue Fix
1 TestSIGTERMDuringSetup sends SIGINT, not SIGTERM Changed os.Interruptsyscall.SIGTERM
2 TestConcurrentAboveLimit 500ms race window Replaced fixed sleep with polling loop that retries the overflow request until 409 (10s timeout)
3 TestSIGTERMDuringSetup 3s race window Replaced fixed sleep with waitForServerStatus() polling /health-check for STARTING status (60s timeout)
7 Weak predict_time assertion Changed from key-existence check to regex verifying a decimal number value
8 Regex . as quote wildcard Added comment explaining this is intentional — testscript single-quoted args cannot embed literal single quotes, so . is the pragmatic choice

Not changing

# Issue Reasoning
4 webhook_delivery_failure exec sleep 5 The real assertion is the subsequent sync prediction succeeding, not the sleep. No reliable poll target exists without knowledge of coglet's internal backoff schedule.
5 ! curl stderr has no consumer Infrastructure code that enables a test pattern. Reasonable to land capability ahead of tests that use it.
6 predict_sys_exit worker identity Distinguishing "same worker survived" from "worker respawned" would require exposing worker PID through the predictor/IPC protocol. The observable behavior (second prediction succeeds) is what the test cares about.
9 Missing [short] skip Checked the broader codebase: most cog serve tests (healthcheck_*, coglet_metrics, coglet_large_input, etc.) do not use [short] skip. Only notably slow tests (GPU builds, large files, subprocess tests) use it. The new tests are consistent with the majority pattern.

Copy link
Member

@michaeldwan michaeldwan left a comment

Choose a reason for hiding this comment

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

looks good, thanks for the fixes!

@markphelps markphelps merged commit 39343a1 into main Mar 17, 2026
31 checks passed
@markphelps markphelps deleted the integration-test-coverage-gaps branch March 17, 2026 18:47
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.

2 participants