Skip to content

ci: Skip 3 consistently timing out tests#42

Closed
derekhiggins wants to merge 1 commit intoopendatahub-io:mainfrom
derekhiggins:skip-timeouts
Closed

ci: Skip 3 consistently timing out tests#42
derekhiggins wants to merge 1 commit intoopendatahub-io:mainfrom
derekhiggins:skip-timeouts

Conversation

@derekhiggins
Copy link
Copy Markdown
Collaborator

@derekhiggins derekhiggins commented Sep 18, 2025

  • test_text_chat_completion_structured_output
  • test_openai_chat_completion_non_streaming
  • test_inference_store

These tests have been consistently timing out and are now skipped in the integration test suite.

Summary by CodeRabbit

  • Tests
    • Expanded the integration test skip list to include additional cases, ensuring the test suite runs smoothly in CI.
    • No changes to application behavior or user-facing functionality.
    • Build and deployment workflows remain unaffected.

- test_text_chat_completion_structured_output
- test_openai_chat_completion_non_streaming
- test_inference_store

These tests have been consistently timing out and are now skipped in the integration test suite.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 18, 2025

Walkthrough

The integration test runner script updates its skip list to include three additional tests. The pytest invocation pattern remains unchanged.

Changes

Cohort / File(s) Summary
Test runner script
tests/run_integration_tests.sh
Extended SKIP_TESTS to also skip test_text_chat_completion_structured_output, test_openai_chat_completion_non_streaming, and test_inference_store; existing skips retained; pytest command unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

I thump my paws on terminal ground,
Three more tests hop past, not to be found.
The runner skips with nimble grace,
Pytest trails the same old pace.
Carrots queued, logs neat and tight—
Green meadows soon, all tests alight! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "ci: Skip 3 consistently timing out tests" is concise and directly describes the primary change — skipping three CI integration tests that have been timing out. It uses a clear scope prefix ("ci:"), avoids noisy details, and matches the PR objective and raw summary so a reviewer can immediately understand the main intent.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
tests/run_integration_tests.sh (2)

43-58: Make skip list configurable and visible in CI logs.

Allow CI to append temporary skips without editing the script and echo what’s being skipped for transparency.

@@
-    # Test to skip
-    SKIP_TESTS="test_text_chat_completion_tool_calling_tools_not_in_request or test_inference_store_tool_calls or test_text_chat_completion_structured_output or test_openai_chat_completion_non_streaming or test_inference_store"
+    # Tests to skip
+    SKIP_TESTS="test_text_chat_completion_tool_calling_tools_not_in_request or test_inference_store_tool_calls or test_text_chat_completion_structured_output or test_openai_chat_completion_non_streaming or test_inference_store"
+    # Allow CI to append more skips via env (e.g., EXTRA_SKIP_TESTS='foo or bar')
+    if [ -n "${EXTRA_SKIP_TESTS:-}" ]; then
+        SKIP_TESTS="$SKIP_TESTS or $EXTRA_SKIP_TESTS"
+    fi
+    echo "Will skip via pytest -k: ($SKIP_TESTS)"

28-31: Nit: fix comment typo.

“incase” → “in case”.

-    # fetch origin incase we didn't clone a fresh repo
+    # fetch origin in case we didn't clone a fresh repo
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b535695 and 2cf15d8.

📒 Files selected for processing (1)
  • tests/run_integration_tests.sh (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build-test-push (linux/amd64)
🔇 Additional comments (1)
tests/run_integration_tests.sh (1)

44-44: Tighten -k skip matching to avoid accidental over-skip.

pytest -k does substring matching on node IDs; e.g., "test_inference_store" can match unrelated nodes. Prefer full node IDs (path/to/file.py::test_name) or stricter -k expressions to avoid accidental over-skip.

File: tests/run_integration_tests.sh: line 44

    SKIP_TESTS="test_text_chat_completion_tool_calling_tools_not_in_request or test_inference_store_tool_calls or test_text_chat_completion_structured_output or test_openai_chat_completion_non_streaming or test_inference_store"

Verification here failed: uv not found. Run locally to confirm what will be skipped:

SKIP_TESTS='test_text_chat_completion_tool_calling_tools_not_in_request or test_inference_store_tool_calls or test_text_chat_completion_structured_output or test_openai_chat_completion_non_streaming or test_inference_store'
pytest -q tests/integration/inference/ --collect-only -k "($SKIP_TESTS)" | sed -n 's/^collected.*$//p; t; s/^<Module //p; s/^  <Function //p'

Copy link
Copy Markdown
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

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

Let's move on until we can to the bottom of this. This is non-critical issue and the CI still offers decent coverage anyway.

@leseb
Copy link
Copy Markdown
Collaborator

leseb commented Sep 18, 2025

Closing for #43

@leseb leseb closed this Sep 18, 2025
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