[workbenches] improve the failure information shown in logs#1166
Merged
jstourac merged 2 commits intoopendatahub-io:mainfrom Mar 6, 2026
Merged
[workbenches] improve the failure information shown in logs#1166jstourac merged 2 commits intoopendatahub-io:mainfrom
jstourac merged 2 commits intoopendatahub-io:mainfrom
Conversation
jstourac
requested review from
andyatmiami,
harshad16 and
jiridanek
as code owners
Contributor
📝 WalkthroughWalkthroughAdds pod-diagnostic helpers and configurable pod readiness timeouts in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/workbenches/conftest.py (1)
55-60: Consider defensive access forpod_status.If
notebook_pod.instance.statusisNone(which can happen for newly created pods), the subsequent_read_obj_fieldcalls will work correctly since they handleNoneobjects. However, you might want to add a guard for consistency with the defensive pattern used elsewhere.♻️ Optional: Add explicit None handling
def _collect_notebook_pod_diagnostics(notebook_pod: Pod) -> str: """Collect concise pod status details for pytest assertion messages.""" - pod_status = notebook_pod.instance.status + pod_status = notebook_pod.instance.status if notebook_pod.instance else None pod_phase = _read_obj_field(obj=pod_status, field_name="phase", default="Unknown")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workbenches/conftest.py` around lines 55 - 60, In _collect_notebook_pod_diagnostics, add an explicit guard for notebook_pod.instance.status being None before calling _read_obj_field: read pod_status = notebook_pod.instance.status and if it's None set a safe empty sentinel (e.g., {}) or short-circuit to return a clear diagnostic string; then proceed to call _read_obj_field (which already tolerates None) to build pod_phase, pod_reason, pod_message. This keeps the defensive pattern consistent with other code paths and uses the existing _read_obj_field helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/workbenches/conftest.py`:
- Around line 55-60: In _collect_notebook_pod_diagnostics, add an explicit guard
for notebook_pod.instance.status being None before calling _read_obj_field: read
pod_status = notebook_pod.instance.status and if it's None set a safe empty
sentinel (e.g., {}) or short-circuit to return a clear diagnostic string; then
proceed to call _read_obj_field (which already tolerates None) to build
pod_phase, pod_reason, pod_message. This keeps the defensive pattern consistent
with other code paths and uses the existing _read_obj_field helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78c3ee93-33f1-48a6-80ec-a1d25083e8ad
📒 Files selected for processing (2)
tests/workbenches/conftest.pytests/workbenches/notebook-controller/test_spawning.py
jiridanek
previously approved these changes
Mar 4, 2026
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/wip', '/verified', '/build-push-pr-image', '/cherry-pick', '/lgtm'} |
jstourac
force-pushed
the
workbenchesBetterFailure
branch
from
17b978e to
3aa7be3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/workbenches/conftest.py (1)
173-183: Make the PVC dependency explicitly intentional indefault_notebook.
users_persistent_volume_claimis used for fixture ordering only. Add an explicit sink assignment to avoid ARG001 noise and clarify intent.Proposed tweak
def default_notebook( request: pytest.FixtureRequest, admin_client: DynamicClient, notebook_image: str, users_persistent_volume_claim: PersistentVolumeClaim, ) -> Generator[Notebook]: @@ - namespace = request.param["namespace"] + _ = users_persistent_volume_claim + namespace = request.param["namespace"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workbenches/conftest.py` around lines 173 - 183, The users_persistent_volume_claim fixture in default_notebook is only used to enforce ordering and is triggering ARG001; make the dependency explicit by adding an intentional sink assignment using the fixture (e.g., assign it to a throwaway variable or comment-backed sink) inside the default_notebook function so linters understand it's intentionally unused; locate the default_notebook function and add a single-line sink assignment referencing users_persistent_volume_claim to clarify intent and suppress ARG001.tests/workbenches/notebook-controller/test_spawning.py (1)
30-36: Silence ARG002 for fixture-only parameters in test methods.These parameters are intentionally present for fixture setup/parametrization, but they are unused in the method body. Add explicit sink assignments to avoid linter warnings.
Minimal no-op usage
def test_create_simple_notebook( self, notebook_pod: Pod, unprivileged_model_namespace: Namespace, users_persistent_volume_claim: PersistentVolumeClaim, default_notebook: Notebook, ): @@ + _ = (unprivileged_model_namespace, users_persistent_volume_claim, default_notebook) assert notebook_pod.exists @@ def test_auth_container_resource_customization( self, notebook_pod: Pod, unprivileged_model_namespace: Namespace, users_persistent_volume_claim: PersistentVolumeClaim, default_notebook: Notebook, ): @@ + _ = (unprivileged_model_namespace, users_persistent_volume_claim, default_notebook) # Verify Auth container has the expected resource values auth_container = self._get_auth_container(pod=notebook_pod)Also applies to: 67-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workbenches/notebook-controller/test_spawning.py` around lines 30 - 36, The test_create_simple_notebook test has unused fixture parameters (notebook_pod, unprivileged_model_namespace, users_persistent_volume_claim, default_notebook) which trigger ARG002; silence the linter by adding explicit sink assignments inside the function (e.g., assign each unused fixture to a throwaway variable like _ = notebook_pod, _ = unprivileged_model_namespace, etc.) so the fixtures are still evaluated but the parameters are marked used; apply the same pattern to the other test function referencing those fixtures (lines 67-73).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/workbenches/conftest.py`:
- Around line 349-360: The timeout handler currently calls
collect_pod_information and _collect_notebook_pod_diagnostics which can raise
and mask the original readiness exception e; wrap those diagnostic calls in
their own try/except so any diagnostic failure is caught and logged (or
ignored), and build pod_diagnostics from a safe fallback string when diagnostics
fail, then raise the AssertionError using _ERR_POD_NOT_READY with the original
exception e as the cause; reference the existing symbols notebook_pod.exists,
collect_pod_information, _collect_notebook_pod_diagnostics, _ERR_POD_NOT_READY,
default_notebook, pod_ready_timeout and ensure the final raise preserves "from
e".
---
Nitpick comments:
In `@tests/workbenches/conftest.py`:
- Around line 173-183: The users_persistent_volume_claim fixture in
default_notebook is only used to enforce ordering and is triggering ARG001; make
the dependency explicit by adding an intentional sink assignment using the
fixture (e.g., assign it to a throwaway variable or comment-backed sink) inside
the default_notebook function so linters understand it's intentionally unused;
locate the default_notebook function and add a single-line sink assignment
referencing users_persistent_volume_claim to clarify intent and suppress ARG001.
In `@tests/workbenches/notebook-controller/test_spawning.py`:
- Around line 30-36: The test_create_simple_notebook test has unused fixture
parameters (notebook_pod, unprivileged_model_namespace,
users_persistent_volume_claim, default_notebook) which trigger ARG002; silence
the linter by adding explicit sink assignments inside the function (e.g., assign
each unused fixture to a throwaway variable like _ = notebook_pod, _ =
unprivileged_model_namespace, etc.) so the fixtures are still evaluated but the
parameters are marked used; apply the same pattern to the other test function
referencing those fixtures (lines 67-73).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33adcf44-571a-4714-bab9-b97dbfdbcfc0
📒 Files selected for processing (2)
tests/workbenches/conftest.pytests/workbenches/notebook-controller/test_spawning.py
jstourac
force-pushed
the
workbenchesBetterFailure
branch
2 times, most recently
from
3073d32 to
07e4ba9
Compare
Member
Author
|
I pushed an update to address your comments and then rebased. @coderabbitai full review |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/workbenches/notebook-controller/test_spawning.py (1)
30-35: Remove unused fixture arguments from test signatures to clear ARG002.The extra args are only dependency triggers. Prefer
usefixturesso the signatures only keep values actually used.♻️ Suggested cleanup
`@pytest.mark.parametrize`( "unprivileged_model_namespace,users_persistent_volume_claim,default_notebook,notebook_pod", @@ indirect=True, ) + `@pytest.mark.usefixtures`("unprivileged_model_namespace", "users_persistent_volume_claim", "default_notebook") def test_create_simple_notebook( self, notebook_pod: Pod, - unprivileged_model_namespace: Namespace, - users_persistent_volume_claim: PersistentVolumeClaim, - default_notebook: Notebook, ): @@ `@pytest.mark.parametrize`( "unprivileged_model_namespace,users_persistent_volume_claim,default_notebook,notebook_pod", @@ indirect=True, ) + `@pytest.mark.usefixtures`("unprivileged_model_namespace", "users_persistent_volume_claim", "default_notebook") def test_auth_container_resource_customization( self, notebook_pod: Pod, - unprivileged_model_namespace: Namespace, - users_persistent_volume_claim: PersistentVolumeClaim, - default_notebook: Notebook, ):Also applies to: 67-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workbenches/notebook-controller/test_spawning.py` around lines 30 - 35, The test function test_create_simple_notebook (and the other test at lines 67-73) currently declares unused fixture parameters (notebook_pod, unprivileged_model_namespace, users_persistent_volume_claim, default_notebook) which triggers ARG002; remove those unused parameters from the test signature and instead attach them via pytest.usefixtures (or the `@pytest.mark.usefixtures` decorator) so the fixtures still run but the function signature only contains values actually used; update the function definitions for test_create_simple_notebook and the other test to drop the unused args and add the appropriate usefixtures decorator referencing the original fixture names.tests/workbenches/conftest.py (2)
323-325: Validate timeout params before passing towait_for_condition.If
request.paramis malformed (non-dict or non-positive timeout), the current flow can fail with less actionable errors.🛠️ Suggested input hardening
- params = getattr(request, "param", {}) - pod_ready_timeout = params.get("timeout", Timeout.TIMEOUT_10MIN) + params = getattr(request, "param", {}) + if params is None: + params = {} + if not isinstance(params, dict): + raise TypeError(f"Expected fixture params to be a dict, got {type(params).__name__}") + + pod_ready_timeout = params.get("timeout", Timeout.TIMEOUT_10MIN) + if not isinstance(pod_ready_timeout, int) or pod_ready_timeout <= 0: + raise ValueError(f"timeout must be a positive integer, got {pod_ready_timeout!r}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workbenches/conftest.py` around lines 323 - 325, Validate and sanitize the timeout input before calling wait_for_condition: ensure request.param is a dict (use params = getattr(request, "param", {}) and if not a dict, replace with {}), extract pod_ready_timeout from params.get("timeout", Timeout.TIMEOUT_10MIN"), verify the extracted pod_ready_timeout is a positive number (int/float > 0) and if not, fall back to Timeout.TIMEOUT_10MIN; use the sanitized pod_ready_timeout when calling wait_for_condition so malformed or negative inputs don't propagate into wait_for_condition.
178-178: Mark the fixture-order dependency as intentionally unused.Line 178 currently triggers ARG001. Since this parameter is for fixture ordering, add an explicit suppression/comment to keep lint noise down.
♻️ Suggested small lint-safe adjustment
def default_notebook( request: pytest.FixtureRequest, admin_client: DynamicClient, notebook_image: str, - users_persistent_volume_claim: PersistentVolumeClaim, + users_persistent_volume_claim: PersistentVolumeClaim, # noqa: ARG001 - fixture ordering dependency ) -> Generator[Notebook]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workbenches/conftest.py` at line 178, The parameter users_persistent_volume_claim in tests/workbenches/conftest.py is intentionally only present for fixture ordering but triggers ARG001; keep the fixture name (don’t rename) and add an explicit inline suppression/comment such as appending " # noqa: ARG001 # unused fixture for ordering" to the users_persistent_volume_claim parameter in the function signature so the linter is silenced and the intent is documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/workbenches/conftest.py`:
- Around line 323-325: Validate and sanitize the timeout input before calling
wait_for_condition: ensure request.param is a dict (use params =
getattr(request, "param", {}) and if not a dict, replace with {}), extract
pod_ready_timeout from params.get("timeout", Timeout.TIMEOUT_10MIN"), verify the
extracted pod_ready_timeout is a positive number (int/float > 0) and if not,
fall back to Timeout.TIMEOUT_10MIN; use the sanitized pod_ready_timeout when
calling wait_for_condition so malformed or negative inputs don't propagate into
wait_for_condition.
- Line 178: The parameter users_persistent_volume_claim in
tests/workbenches/conftest.py is intentionally only present for fixture ordering
but triggers ARG001; keep the fixture name (don’t rename) and add an explicit
inline suppression/comment such as appending " # noqa: ARG001 # unused fixture
for ordering" to the users_persistent_volume_claim parameter in the function
signature so the linter is silenced and the intent is documented.
In `@tests/workbenches/notebook-controller/test_spawning.py`:
- Around line 30-35: The test function test_create_simple_notebook (and the
other test at lines 67-73) currently declares unused fixture parameters
(notebook_pod, unprivileged_model_namespace, users_persistent_volume_claim,
default_notebook) which triggers ARG002; remove those unused parameters from the
test signature and instead attach them via pytest.usefixtures (or the
`@pytest.mark.usefixtures` decorator) so the fixtures still run but the function
signature only contains values actually used; update the function definitions
for test_create_simple_notebook and the other test to drop the unused args and
add the appropriate usefixtures decorator referencing the original fixture
names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d624d3c-67ba-442e-81a0-c80bd648799b
📒 Files selected for processing (2)
tests/workbenches/conftest.pytests/workbenches/notebook-controller/test_spawning.py
jstourac
force-pushed
the
workbenchesBetterFailure
branch
from
07e4ba9 to
9d4de82
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/workbenches/conftest.py (1)
179-185: Consider suppressing the static analysis warning for clarity.The
users_persistent_volume_claimparameter is intentionally unused—it ensures fixture ordering so the PVC exists before notebook creation. The docstring documents this well, but consider adding# noqa: ARG001to explicitly signal to static analysis tools and future readers that this is intentional.def default_notebook( request: pytest.FixtureRequest, admin_client: DynamicClient, unprivileged_client: DynamicClient, notebook_image: str, - users_persistent_volume_claim: PersistentVolumeClaim, + users_persistent_volume_claim: PersistentVolumeClaim, # noqa: ARG001 # ensures PVC exists first ) -> Generator[Notebook]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workbenches/conftest.py` around lines 179 - 185, Add an inline suppression for the unused fixture parameter by appending a flake8/ruff ignore to the parameter: update the function signature so the parameter users_persistent_volume_claim: PersistentVolumeClaim has a trailing comment "# noqa: ARG001" (or the project's preferred lint code) to explicitly mark it as intentionally unused; keep the existing docstring and ordering behavior unchanged so fixture ordering still guarantees the PVC exists before Notebook creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/workbenches/conftest.py`:
- Around line 179-185: Add an inline suppression for the unused fixture
parameter by appending a flake8/ruff ignore to the parameter: update the
function signature so the parameter users_persistent_volume_claim:
PersistentVolumeClaim has a trailing comment "# noqa: ARG001" (or the project's
preferred lint code) to explicitly mark it as intentionally unused; keep the
existing docstring and ordering behavior unchanged so fixture ordering still
guarantees the PVC exists before Notebook creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3bffdce0-5e2b-4e05-bcf5-c7e8766707f1
📒 Files selected for processing (2)
tests/workbenches/conftest.pytests/workbenches/notebook-controller/test_spawning.py
dbasunag
approved these changes
Mar 5, 2026
jiridanek
approved these changes
Mar 6, 2026
* adds a configurable timeout parameter * adds a dependency to require PVC before workbench creation * includes basic pod diagnostic directly into the log output, e.g.: ``` E Pod diagnostics: E Pod phase=Pending, reason=None, message=None E Condition PodScheduled: status=False, reason=Unschedulable, message=0/6 nodes are \ available: persistentvolumeclaim "test-auth-notebook" not found. not found ```
jstourac
force-pushed
the
workbenchesBetterFailure
branch
from
9d4de82 to
dc35486
Compare
|
Status of building tag latest: success. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
Before this:
After this:
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit
Tests
Refactor