Repository navigation
Conversation
Tests cover: - PATCH debugMode=true/false - PATCH hiddenSidebarItems with array values - Combined updates with both fields
Tests cover: - Debug mode toggle on/off - Sidebar visibility toggle - Settings persistence after page reload
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new end-to-end tests for settings toggles and unit tests for the settings API. The review feedback highlights several critical issues: unit tests contain incorrect assertions and missing await calls for asynchronous functions, and they lack proper state isolation between tests. Additionally, the E2E tests should be refactored to remove conditional logic and hardcoded timeouts, utilizing Playwright's web-first assertions instead for better reliability.
| const result = await updateSettings({ debugMode: true }); | ||
| assert.strictEqual(result.ok, true, "updateSettings should return ok: true"); | ||
|
|
||
| const settings = getSettings(); | ||
| assert.strictEqual(settings.debugMode, true, "debugMode should be true"); |
There was a problem hiding this comment.
This test has two correctness issues that will cause it to fail:
updateSettingsreturns the updated settings object directly (as seen insrc/app/api/settings/route.ts), soresult.okwill beundefinedand the assertion will fail.getSettings()is an asynchronous function but is called here withoutawait. This returns a Promise, makingsettings.debugModeundefined.
This pattern is repeated throughout the file and should be corrected in all test cases.
| const result = await updateSettings({ debugMode: true }); | |
| assert.strictEqual(result.ok, true, "updateSettings should return ok: true"); | |
| const settings = getSettings(); | |
| assert.strictEqual(settings.debugMode, true, "debugMode should be true"); | |
| const result = await updateSettings({ debugMode: true }); | |
| assert.strictEqual(result.debugMode, true, "updateSettings should return the updated settings"); | |
| const settings = await getSettings(); | |
| assert.strictEqual(settings.debugMode, true, "debugMode should be true"); |
| if (await debugToggle.isVisible({ timeout: 3000 }).catch(() => false)) { | ||
| const initialState = await debugToggle.isChecked(); | ||
| await debugToggle.click(); | ||
| await page.waitForTimeout(500); | ||
| const newState = await debugToggle.isChecked(); | ||
| expect(newState).not.toBe(initialState); | ||
| } |
There was a problem hiding this comment.
Avoid using conditional logic (if) to wrap assertions in E2E tests. If the element is not found, the test should fail to indicate a regression or UI change. Additionally, replace page.waitForTimeout with Playwright's web-first assertions like toBeChecked(), which automatically wait for the expected state and are more resilient to timing issues.
await expect(debugToggle).toBeVisible();
const initialState = await debugToggle.isChecked();
await debugToggle.click();
await expect(debugToggle).toBeChecked({ checked: !initialState });| import assert from "node:assert/strict"; | ||
| import { getSettings, updateSettings } from "../../src/lib/localDb.ts"; | ||
|
|
||
| describe("Settings API - debugMode and hiddenSidebarItems", () => { |
There was a problem hiding this comment.
The unit tests modify the global settings state in the database without resetting it between tests. This can lead to test interference and non-deterministic results. Consider adding a beforeEach hook to reset the settings to a default state before each test.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Resolved IssuesThe following issues from the previous review have been fixed:
Files Reviewed (4 files)
Fix Link |
- Unit tests: fix async/await for getSettings, use direct db functions - E2E tests: remove conditional logic, use Playwright auto-waiting assertions
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| import assert from "node:assert/strict"; | ||
| import { getSettings, updateSettings } from "../../src/lib/db/settings.ts"; | ||
|
|
||
| describe("Settings API - debugMode and hiddenSidebarItems", () => { |
There was a problem hiding this comment.
WARNING: Tests modify global settings state without resetting between tests - this can cause test interference and non-deterministic results. Consider adding a beforeEach hook to reset settings to a default state before each test.
Add section documenting: - Debug Mode toggle (Settings → Advanced) - Sidebar Visibility toggle (Settings → General)
Only inject prompt_cache_key for providers that support prompt caching (Claude, Anthropic, ZAI, Qwen, DeepSeek). This fixes issue cheaper-inference#848 where NVIDIA API rejected the parameter.
|
|
||
| await expect(debugToggle).toBeVisible({ timeout: 5000 }); | ||
|
|
||
| const initialState = await debugToggle.isChecked(); |
There was a problem hiding this comment.
SUGGESTION: Unused variable initialState - consider removing or using it for assertions
|
|
||
| await expect(sidebarToggle).toBeVisible({ timeout: 5000 }); | ||
|
|
||
| const initialState = await sidebarToggle.isChecked(); |
There was a problem hiding this comment.
SUGGESTION: Unused variable initialState - consider removing or using it for assertions
|
|
||
| await expect(debugToggle).toBeVisible({ timeout: 5000 }); | ||
|
|
||
| const wasChecked = await debugToggle.isChecked(); |
There was a problem hiding this comment.
SUGGESTION: Unused variable wasChecked - consider removing or using it for assertions
|
Thanks @oyi77 for this great contribution! 🎉 The improvements have been integrated into the |
Summary
Add tests for debug mode and sidebar visibility settings toggles, plus bug fix for issue #848.
Files Changed
prompt_cache_key[nvidia] #848Test Coverage
Bug Fix Issue #848
Related
prompt_cache_key[nvidia] #848