test: replace test_should_ naming with test_verb_noun_context convention#44
test: replace test_should_ naming with test_verb_noun_context convention#44aivong-openhands wants to merge 17 commits into
Conversation
The test_should_* pattern in TestRecaptchaServiceHashAccountId, TestRecaptchaServiceCreateAssessment, and TestKeycloakCallbackRecaptcha diverged from the test_verb_noun_context style used throughout the rest of the suite. Rename 23 tests across two files: - enterprise/tests/unit/test_recaptcha_service.py (13 renames) - enterprise/tests/unit/test_auth_routes.py (10 renames) No test logic changed. Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughA mixed change set: test renames and docstring additions; multiple GitHub Actions version bumps; addition of an LLM base URL resolver with corresponding code and unit tests; small frontend queryKey and UI layout tweaks; dependency version bump and assorted documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UsersRoute as Users /api/v1/users/me
participant LLMResolver as resolve_provider_llm_base_url
participant EnvProvider as Provider config/env
Client->>UsersRoute: GET /api/v1/users/me (includes agent_settings.llm)
UsersRoute->>LLMResolver: resolve_provider_llm_base_url(model, llm.base_url)
LLMResolver->>EnvProvider: read provider base (OPENHANDS_PROVIDER_BASE_URL / LLM_BASE_URL) if needed
EnvProvider-->>LLMResolver: provider_base_url or none
LLMResolver-->>UsersRoute: effective llm_base_url
UsersRoute-->>Client: response including resolved llm_base_url
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sers/me (OpenHands#13980) Co-authored-by: openhands <openhands@all-hands.dev> Co-authored-by: hieptl <hieptl.developer@gmail.com>
Co-authored-by: OpenHands CVE Fix Bot <openhands@all-hands.dev>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/review |
…penHands#13996) Co-authored-by: octo-patch <octo-patch@github.com> Co-authored-by: Graham Neubig <neubig@gmail.com>
Co-authored-by: openhands <openhands@all-hands.dev>
…OpenHands#13931) Co-authored-by: Test Admin <ai.vong+bdctestadmin@openhands.dev> Co-authored-by: Claude <noreply@anthropic.com>
…3761) Co-authored-by: openhands <openhands@all-hands.dev>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Engel Nyst <engel.nyst@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/__tests__/hooks/mutation/use-add-git-providers.test.tsx (2)
70-73: Consider asserting the exact invalidated query key.Currently the test only checks that
invalidateQuerieswas called and that the pre-seeded personal settings entry is invalidated. Since the production change is specifically about targeting the more specific key["settings", "personal", organizationId], asserting the argument would catch regressions where the key drifts back to["settings", organizationId](which would still invalidate the seeded entry via prefix matching and pass the current assertion).♻️ Suggested strengthening
- expect(invalidateSpy).toHaveBeenCalled(); + expect(invalidateSpy).toHaveBeenCalledWith({ + queryKey: ["settings", "personal", "org-1"], + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/hooks/mutation/use-add-git-providers.test.tsx` around lines 70 - 73, Test currently only verifies invalidateQueries was called and that personalSettingsQueryKey state is invalidated; strengthen it to assert the exact key argument used when calling invalidateQueries to ensure it's targeting ["settings","personal", organizationId] not a broader prefix. Update the test to check invalidateSpy was calledWith the specific query key (reference invalidateSpy and the invalidateQueries call) and assert the argument equals the expected ["settings","personal", organizationId] (use the same organizationId variable used when seeding personalSettingsQueryKey) so regressions reverting to ["settings", organizationId] are caught.
34-46: Optional: add coverage for tracking behavior.The suite only exercises the invalidation path. A second case asserting
trackGitProviderConnectedis called with non-empty tokens and skipped when all tokens are empty would round out coverage of theonSuccesshandler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/hooks/mutation/use-add-git-providers.test.tsx` around lines 34 - 46, Add tests in the useAddGitProviders suite to cover the onSuccess tracking behavior: create one test that calls the mutation with a provider response containing at least one non-empty token and assert mockTrackGitProviderConnected was called with the expected provider key(s) (and that queryClient invalidation still occurs), and a second test that calls the mutation with a response where all token fields are empty/falsey and assert mockTrackGitProviderConnected was not called. Target the same setup used in the file (useAddGitProviders hook, queryClient instance, and mockTrackGitProviderConnected) and invoke the mutation's onSuccess path (resolve the mutation promise or call the hook's mutate/mutateAsync) to trigger the handler in each case.openhands/app_server/config.py (1)
121-156: Consider documentingprovider_base_url=''vsNonesemantics.The resolver treats
provider_base_url=Noneas "fall back to env lookup" and any falsy value ('', etc.) as "no provider available, returnbase_url". This subtle distinction is leveraged in tests (e.g.,provider_base_url=''to suppress env lookup) but isn't called out in the docstring. Callers passing a cached provider URL from config (likelive_status_app_conversation_service.py, which storesNonewhen env is unset) will trigger a redundant env re-read on every call — harmless, but worth a note.📝 Suggested docstring tweak
Args: model: LLM model name (e.g. ``litellm_proxy/gpt-4``). base_url: The base URL from user/org settings. - provider_base_url: Deployment provider URL. Falls back to - ``get_openhands_provider_base_url()`` when *None*. + provider_base_url: Deployment provider URL. When ``None`` (default), + falls back to ``get_openhands_provider_base_url()`` (env lookup). + Pass an empty string to explicitly suppress the env fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openhands/app_server/config.py` around lines 121 - 156, Update the resolve_provider_llm_base_url docstring to explicitly describe the semantics of provider_base_url: None means "perform env lookup via get_openhands_provider_base_url()", while any falsy but non-None value (e.g. empty string '') means "suppress env lookup and treat as no provider available (return base_url)"; mention callers that cache provider URLs should store and pass '' to avoid repeated env reads or pass the cached value (None only if they want env re-evaluated). Reference resolve_provider_llm_base_url and get_openhands_provider_base_url in the docstring so future readers and callers (e.g., live_status_app_conversation_service.py) understand the intended behavior..github/workflows/ghcr-build.yml (1)
111-111: Workflow version bumps look valid, but are out of scope for this PR.The PR title/objectives scope this change to test renaming (
test_should_*→test_verb_noun_context), yet this file only contains GitHub Actions version bumps (docker/login-action@v4,docker/build-push-action@v7,docker/metadata-action@v6at lines 111, 205, 218, 239, 305, 327, 353, 378). These versions exist and the inputs used here (push,tags,platforms,cache-from/to,build-args,context,provenance,sbom, metadatatags/flavor) remain supported, so the changes themselves are safe — but mixing infra bumps with a pure test-naming refactor makes the PR harder to review and revert. Consider splitting workflow/action bumps into a separate PR.Note:
docker/build-push-action@v7removes the deprecatedDOCKER_BUILD_NO_SUMMARY/DOCKER_BUILD_EXPORT_RETENTION_DAYSenvs and theinstallinput — none of which are used here, so no functional impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ghcr-build.yml at line 111, This PR mixes test renaming with GitHub Actions version bumps; revert the action version changes in this branch by restoring the previous action refs (e.g., docker/login-action@v3, docker/build-push-action@v6, docker/metadata-action@v5) for the workflow entries that were updated (look for uses: docker/login-action@v4, docker/build-push-action@v7, docker/metadata-action@v6) and move those upgrade changes into a separate branch/PR dedicated to infra bumps so the test renaming PR stays focused..github/workflows/openhands-resolver.yml (1)
195-195: Document minimum runner requirements foractions/github-script@v9compatibility.This reusable workflow accepts
inputs.runner, allowing callers to specify custom runners. However,actions/github-script@v9requires Node 24 runtime and GitHub Actions runner version v2.327.1 or later. Consider documenting these minimum requirements in the workflow or comments to prevent caller surprises when using older self-hosted runners.Also applies to: 209-209, 308-308, 344-344, 419-419
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/openhands-resolver.yml at line 195, The workflow uses actions/github-script@v9 in steps that accept the inputs.runner override (reference the uses: actions/github-script@v9 occurrences and the inputs.runner parameter); add documentation/comments in the reusable workflow header (or a README block) that clearly state the minimum runtime and runner requirements: Node 24 (Node.js v24) and GitHub Actions runner version v2.327.1 or later, and call out that self-hosted runners must meet these versions; update the comment near each actions/github-script@v9 occurrence (the lines flagged) so callers see the requirement when overriding inputs.runner.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/welcome-good-first-issue.yml:
- Line 17: The PR mixes a test-name refactor with a breaking dependency bump:
multiple workflow steps using the action identifier "uses:
actions/github-script@v9" were introduced here; either revert those steps back
to the prior pin (e.g., actions/github-script@v7) in this PR or move all
occurrences of "uses: actions/github-script@v9" into a separate PR that upgrades
the action, and update the PR description to explicitly state the workflow
upgrade and include a validation plan (manual workflow run or CI workflow tests)
verifying the embedded scripts still work with v9 before landing.
---
Nitpick comments:
In @.github/workflows/ghcr-build.yml:
- Line 111: This PR mixes test renaming with GitHub Actions version bumps;
revert the action version changes in this branch by restoring the previous
action refs (e.g., docker/login-action@v3, docker/build-push-action@v6,
docker/metadata-action@v5) for the workflow entries that were updated (look for
uses: docker/login-action@v4, docker/build-push-action@v7,
docker/metadata-action@v6) and move those upgrade changes into a separate
branch/PR dedicated to infra bumps so the test renaming PR stays focused.
In @.github/workflows/openhands-resolver.yml:
- Line 195: The workflow uses actions/github-script@v9 in steps that accept the
inputs.runner override (reference the uses: actions/github-script@v9 occurrences
and the inputs.runner parameter); add documentation/comments in the reusable
workflow header (or a README block) that clearly state the minimum runtime and
runner requirements: Node 24 (Node.js v24) and GitHub Actions runner version
v2.327.1 or later, and call out that self-hosted runners must meet these
versions; update the comment near each actions/github-script@v9 occurrence (the
lines flagged) so callers see the requirement when overriding inputs.runner.
In `@frontend/__tests__/hooks/mutation/use-add-git-providers.test.tsx`:
- Around line 70-73: Test currently only verifies invalidateQueries was called
and that personalSettingsQueryKey state is invalidated; strengthen it to assert
the exact key argument used when calling invalidateQueries to ensure it's
targeting ["settings","personal", organizationId] not a broader prefix. Update
the test to check invalidateSpy was calledWith the specific query key (reference
invalidateSpy and the invalidateQueries call) and assert the argument equals the
expected ["settings","personal", organizationId] (use the same organizationId
variable used when seeding personalSettingsQueryKey) so regressions reverting to
["settings", organizationId] are caught.
- Around line 34-46: Add tests in the useAddGitProviders suite to cover the
onSuccess tracking behavior: create one test that calls the mutation with a
provider response containing at least one non-empty token and assert
mockTrackGitProviderConnected was called with the expected provider key(s) (and
that queryClient invalidation still occurs), and a second test that calls the
mutation with a response where all token fields are empty/falsey and assert
mockTrackGitProviderConnected was not called. Target the same setup used in the
file (useAddGitProviders hook, queryClient instance, and
mockTrackGitProviderConnected) and invoke the mutation's onSuccess path (resolve
the mutation promise or call the hook's mutate/mutateAsync) to trigger the
handler in each case.
In `@openhands/app_server/config.py`:
- Around line 121-156: Update the resolve_provider_llm_base_url docstring to
explicitly describe the semantics of provider_base_url: None means "perform env
lookup via get_openhands_provider_base_url()", while any falsy but non-None
value (e.g. empty string '') means "suppress env lookup and treat as no provider
available (return base_url)"; mention callers that cache provider URLs should
store and pass '' to avoid repeated env reads or pass the cached value (None
only if they want env re-evaluated). Reference resolve_provider_llm_base_url and
get_openhands_provider_base_url in the docstring so future readers and callers
(e.g., live_status_app_conversation_service.py) understand the intended
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 772e457c-953f-4f38-a1c1-000a01433b0d
⛔ Files ignored due to path filters (3)
enterprise/poetry.lockis excluded by!**/*.lockpoetry.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/ghcr-build.yml.github/workflows/openhands-resolver.yml.github/workflows/pr-artifacts.yml.github/workflows/py-tests.yml.github/workflows/welcome-good-first-issue.ymlAGENTS.mdenterprise/server/routes/users_v1.pyenterprise/tests/unit/test_maintenance_task_runner_standalone.pyfrontend/__tests__/hooks/mutation/use-add-git-providers.test.tsxfrontend/__tests__/router.mdfrontend/src/hooks/mutation/use-add-git-providers.tsfrontend/src/routes/mcp-settings.tsxopenhands/agenthub/README.mdopenhands/app_server/README.mdopenhands/app_server/app_conversation/live_status_app_conversation_service.pyopenhands/app_server/config.pypyproject.tomltests/unit/app_server/test_resolve_provider_llm_base_url.py
✅ Files skipped from review due to trivial changes (8)
- pyproject.toml
- AGENTS.md
- .github/workflows/py-tests.yml
- frontend/tests/router.md
- openhands/app_server/README.md
- openhands/agenthub/README.md
- enterprise/tests/unit/test_maintenance_task_runner_standalone.py
- frontend/src/routes/mcp-settings.tsx
| - name: Check if welcome comment already exists | ||
| id: check_comment | ||
| uses: actions/github-script@v7 | ||
| uses: actions/github-script@v9 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the upgraded github-script blocks do not use v9-incompatible patterns.
# Expected: matches for actions/github-script@v9 are OK; no matches for require('@actions/github')
# or `const/let getOctokit` redeclarations should appear.
rg -n -C2 "actions/github-script@v9|require\\(['\"]@actions/github['\"]\\)|\\b(const|let)\\s+getOctokit\\b" .github/workflows/welcome-good-first-issue.yml .github/workflows/openhands-resolver.yml .github/workflows/pr-artifacts.ymlRepository: aivong-openhands/OpenHands
Length of output: 4048
🏁 Script executed:
#!/bin/bash
# Check git diff to see what changed in workflow files
git diff HEAD -- .github/workflows/welcome-good-first-issue.yml .github/workflows/openhands-resolver.yml .github/workflows/pr-artifacts.yml | head -200Repository: aivong-openhands/OpenHands
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Get full content of the changed workflow file to understand context
cat -n .github/workflows/welcome-good-first-issue.ymlRepository: aivong-openhands/OpenHands
Length of output: 2759
🏁 Script executed:
#!/bin/bash
# Check git log for recent changes to the workflow files
git log --oneline -10 -- .github/workflows/welcome-good-first-issue.yml .github/workflows/openhands-resolver.yml .github/workflows/pr-artifacts.ymlRepository: aivong-openhands/OpenHands
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Check what version was in the previous commit
git show HEAD~1:.github/workflows/welcome-good-first-issue.yml 2>/dev/null | grep -n "actions/github-script" || echo "File or pattern not found in previous commit"Repository: aivong-openhands/OpenHands
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Check git status to see which files are staged/modified
git status --shortRepository: aivong-openhands/OpenHands
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check what's in HEAD for the workflow file
git show HEAD:.github/workflows/welcome-good-first-issue.yml | grep -n "actions/github-script"Repository: aivong-openhands/OpenHands
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Check parent commit
git log --oneline -3 -- .github/workflows/welcome-good-first-issue.ymlRepository: aivong-openhands/OpenHands
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Check if the file was modified in any of the recent commits
git show HEAD -- .github/workflows/welcome-good-first-issue.yml | head -50Repository: aivong-openhands/OpenHands
Length of output: 1569
🏁 Script executed:
#!/bin/bash
# Check what files were actually changed in this PR/branch
git diff main...HEAD --name-statusRepository: aivong-openhands/OpenHands
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Get a summary of the PR changes
git log --oneline main..HEAD | head -20Repository: aivong-openhands/OpenHands
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Check the branches and their relationship
git branch -aRepository: aivong-openhands/OpenHands
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check what changed in the merge commit specifically
git show 988ec4aa --statRepository: aivong-openhands/OpenHands
Length of output: 50383
🏁 Script executed:
#!/bin/bash
# Look at the full diff of the merge commit
git show 988ec4aa | head -300Repository: aivong-openhands/OpenHands
Length of output: 14238
🏁 Script executed:
#!/bin/bash
# Check which workflow files were actually modified/added in this PR
git diff --name-status HEAD~1 HEAD -- .github/workflows/ 2>/dev/null || echo "No diff available with HEAD~1"
# Check all workflow files added/modified compared to main
git diff --name-status origin/main...HEAD -- .github/workflows/ 2>/dev/null || echo "Cannot compare to origin/main"Repository: aivong-openhands/OpenHands
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Check if openhands-resolver.yml and pr-artifacts.yml were modified
git log -p --follow -1 -- .github/workflows/openhands-resolver.yml | head -50Repository: aivong-openhands/OpenHands
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Get the commit message to understand PR scope
git log --format=%B -1 988ec4aaRepository: aivong-openhands/OpenHands
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Check if welcome-good-first-issue.yml existed in origin/main
git show origin/main:.github/workflows/welcome-good-first-issue.yml 2>&1 | head -20Repository: aivong-openhands/OpenHands
Length of output: 483
🏁 Script executed:
#!/bin/bash
# List all files in the current branch's .github/workflows
ls -la .github/workflows/ | grep -E "welcome|good"Repository: aivong-openhands/OpenHands
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Check version in origin/main for the other workflow files
echo "=== openhands-resolver.yml in origin/main ==="
git show origin/main:.github/workflows/openhands-resolver.yml 2>/dev/null | grep -n "actions/github-script" | head -5
echo "=== pr-artifacts.yml in origin/main ==="
git show origin/main:.github/workflows/pr-artifacts.yml 2>/dev/null | grep -n "actions/github-script" | head -5Repository: aivong-openhands/OpenHands
Length of output: 467
🏁 Script executed:
#!/bin/bash
# Get the actual changes to welcome-good-first-issue.yml
git diff origin/main...HEAD -- .github/workflows/welcome-good-first-issue.yml | head -100Repository: aivong-openhands/OpenHands
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Check the PR description or branch info to understand scope
git log --all --format=%B --grep="test" --grep="naming" -i | head -50Repository: aivong-openhands/OpenHands
Length of output: 1819
Split this major workflow upgrade from the test-name refactor.
The stated PR scope indicates test-name changes, but this PR upgrades actions/github-script from v7 to v9 across three workflow files (9 total uses). This is a major version bump with breaking changes documented in the v9.0.0 release notes. While the embedded scripts in these workflows are compatible with v9 (using only injected github and context objects), the dependency upgrade should either be a separate PR or the PR description/testing plan should explicitly cover workflow validation for this change.
Also applies to: lines 36, .github/workflows/openhands-resolver.yml (lines 195, 209, 308, 344, 419), .github/workflows/pr-artifacts.yml (lines 62, 110)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/welcome-good-first-issue.yml at line 17, The PR mixes a
test-name refactor with a breaking dependency bump: multiple workflow steps
using the action identifier "uses: actions/github-script@v9" were introduced
here; either revert those steps back to the prior pin (e.g.,
actions/github-script@v7) in this PR or move all occurrences of "uses:
actions/github-script@v9" into a separate PR that upgrades the action, and
update the PR description to explicitly state the workflow upgrade and include a
validation plan (manual workflow run or CI workflow tests) verifying the
embedded scripts still work with v9 before landing.
7 participants
Why
Two files used a
test_should_*naming style. This is a legitimate BDD-influenced convention (borrowed from RSpec/Jasmine) and is not wrong. However, the rest of the enterprise test suite usestest_verb_noun_context, so these files stand out. This is a purely stylistic alignment — either convention applied uniformly would be equally valid.Summary
test_recaptcha_service.py(test_should_*→test_verb_noun_context)test_auth_routes.py(TestKeycloakCallbackRecaptchaclass, same rename pattern)Issue Number
N/A
How to Test
Type
Notes
If the team prefers
test_should_*as the standard going forward, this PR should be closed and the other files aligned to that pattern instead.Summary by CodeRabbit