Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tests/e2e-playwright/TEST_PLAN.md
Original file line number Diff line number Diff line change
Expand Up @@ -673,8 +673,8 @@ The test plan is organized by feature area. Tests are grouped for parallel execu
### 7.3 Workbench Settings
| Status | Group | Test Case |
|--------|-------|-----------|
| 🔲 | main | Show editor cleanup switch |
| 🔲 | main | Show pipeline commands setting |
| | main | Show editor cleanup switch |
| | main | Show pipeline commands setting |
| 🔲 | main | Configure command timeout (N/A - per-database setting, not in settings page) |

### 7.4 Redis Cloud Settings
Expand Down
71 changes: 55 additions & 16 deletions tests/e2e-playwright/pages/settings/SettingsPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ export class SettingsPage extends BasePage {
// Workbench settings
readonly editorCleanupSwitch: Locator;
readonly pipelineCommandsText: Locator;
readonly pipelineCommandsValue: Locator;
readonly pipelineCommandsInput: Locator;
readonly pipelineApplyButton: Locator;

// Advanced settings
readonly advancedWarning: Locator;
Expand All @@ -51,12 +54,12 @@ export class SettingsPage extends BasePage {
// Page title
this.pageTitle = page.locator('[data-testid="settings-page-title"]').or(page.getByText('Settings').first());

// Accordion buttons
this.generalButton = page.getByRole('button', { name: 'General' });
this.privacyButton = page.getByRole('button', { name: 'Privacy' });
this.workbenchButton = page.getByRole('button', { name: 'Workbench' });
this.redisCloudButton = page.getByRole('button', { name: 'Redis Cloud', exact: true });
this.advancedButton = page.getByRole('button', { name: 'Advanced' });
// Accordion headings (clicking the heading label toggles the section)
this.generalButton = page.getByRole('heading', { name: 'General' });
this.privacyButton = page.getByRole('heading', { name: 'Privacy' });
this.workbenchButton = page.getByRole('heading', { name: 'Workbench' });
this.redisCloudButton = page.getByRole('heading', { name: 'Redis Cloud', exact: true });
this.advancedButton = page.getByRole('heading', { name: 'Advanced' });

// General settings
this.themeDropdown = page.getByRole('combobox', { name: /color theme/i });
Expand Down Expand Up @@ -88,6 +91,9 @@ export class SettingsPage extends BasePage {
.locator('[data-testid="switch-workbench-cleanup"]')
.or(page.getByRole('switch').filter({ hasText: /Clear the Editor/i }));
this.pipelineCommandsText = page.getByText(/Commands in pipeline/i);
this.pipelineCommandsValue = page.getByTestId('pipeline-bunch-value');
this.pipelineCommandsInput = page.getByTestId('pipeline-bunch-input');
this.pipelineApplyButton = page.getByTestId('apply-btn');
Copy link

Choose a reason for hiding this comment

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

Non-scoped apply-btn locator matches multiple components

Low Severity

The pipelineApplyButton locator uses page.getByTestId('apply-btn'), which is a page-wide selector for a generic test ID shared by every InlineItemEditor instance in the app. The Advanced settings section also uses a SettingItem (which wraps InlineItemEditor with its own apply-btn). If both sections are ever expanded and one enters edit mode unexpectedly, this locator could match the wrong button. A scoped locator — e.g., chaining off the pipeline container — would be more resilient.

Fix in Cursor Fix in Web


// Advanced settings
this.advancedWarning = page.getByRole('alert').filter({ hasText: /Advanced settings/i });
Expand Down Expand Up @@ -148,32 +154,32 @@ export class SettingsPage extends BasePage {
* Check if General section is expanded
*/
async isGeneralExpanded(): Promise<boolean> {
const expanded = await this.generalButton.getAttribute('aria-expanded');
return expanded === 'true';
const state = await this.page.locator('[data-test-subj="accordion-appearance"]').getAttribute('data-state');
return state === 'open';
}

/**
* Check if Privacy section is expanded
*/
async isPrivacyExpanded(): Promise<boolean> {
const expanded = await this.privacyButton.getAttribute('aria-expanded');
return expanded === 'true';
const state = await this.page.locator('[data-test-subj="accordion-privacy-settings"]').getAttribute('data-state');
return state === 'open';
}

/**
* Check if Workbench section is expanded
*/
async isWorkbenchExpanded(): Promise<boolean> {
const expanded = await this.workbenchButton.getAttribute('aria-expanded');
return expanded === 'true';
const state = await this.page.locator('[data-test-subj="accordion-workbench-settings"]').getAttribute('data-state');
return state === 'open';
}

/**
* Check if Advanced section is expanded
*/
async isAdvancedExpanded(): Promise<boolean> {
const expanded = await this.advancedButton.getAttribute('aria-expanded');
return expanded === 'true';
const state = await this.page.locator('[data-test-subj="accordion-advanced-settings"]').getAttribute('data-state');
return state === 'open';
}

/**
Expand All @@ -188,8 +194,8 @@ export class SettingsPage extends BasePage {
* Check if Redis Cloud section is expanded
*/
async isRedisCloudExpanded(): Promise<boolean> {
const expanded = await this.redisCloudButton.getAttribute('aria-expanded');
return expanded === 'true';
const state = await this.page.locator('[data-test-subj="accordion-cloud-settings"]').getAttribute('data-state');
return state === 'open';
}

/**
Expand Down Expand Up @@ -239,4 +245,37 @@ export class SettingsPage extends BasePage {
const checked = await this.notificationSwitch.getAttribute('aria-checked');
return checked === 'true';
}

/**
* Toggle editor cleanup switch
*/
async toggleEditorCleanup(): Promise<void> {
await this.editorCleanupSwitch.click();
}

/**
* Check if editor cleanup is enabled (switch is on)
*/
async isEditorCleanupEnabled(): Promise<boolean> {
const checked = await this.editorCleanupSwitch.getAttribute('aria-checked');
return checked === 'true';
}

/**
* Get current pipeline commands value (displayed number)
*/
async getPipelineCommandsValue(): Promise<string> {
return (await this.pipelineCommandsValue.textContent()) ?? '';
}

/**
* Set pipeline commands value and apply (enters edit mode, fills input, clicks Apply)
*/
async setPipelineCommandsAndApply(value: number): Promise<void> {
await this.pipelineCommandsValue.click();
await this.pipelineCommandsInput.waitFor({ state: 'visible' });
await this.pipelineCommandsInput.clear();
await this.pipelineCommandsInput.fill(String(value));
await this.pipelineApplyButton.click();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { test, expect } from '../../../../fixtures/base';

/**
* Workbench Settings tests (TEST_PLAN.md: 7.3 Workbench Settings)
*
* Verifies Workbench section controls are visible and that changing
* editor cleanup and pipeline commands settings persists after navigation.
*
* Note: "Configure command timeout" is N/A -- it's a per-database setting, not on the Settings page.
*/
test.describe('Workbench Settings', () => {
test.beforeEach(async ({ settingsPage }) => {
await settingsPage.goto();
});

test('should show editor cleanup switch', async ({ settingsPage }) => {
await settingsPage.expandWorkbench();
await expect(settingsPage.editorCleanupSwitch).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

haven't tested it, but if it's easy, it makes sense to add a test that executes something in the workbench so we can validate the switch logic

});

test('should show pipeline commands setting', async ({ settingsPage }) => {
await settingsPage.expandWorkbench();
await expect(settingsPage.pipelineCommandsText).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

and for that one too (I imagine we can set it to 2, and validate just a single command is visible)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think these should be tested here. The test must verify that you can change and save settings. How they affect the related feature, that should be tested as part of that feautre's tests

});

test('should save editor cleanup when toggled', async ({
settingsPage,
databasesPage,
}) => {
await settingsPage.expandWorkbench();
await expect(settingsPage.editorCleanupSwitch).toBeVisible();

const initialState = await settingsPage.isEditorCleanupEnabled();
await settingsPage.toggleEditorCleanup();
const toggledState = await settingsPage.isEditorCleanupEnabled();
expect(toggledState).toBe(!initialState);

await databasesPage.goto();
await settingsPage.goto();
await settingsPage.expandWorkbench();
const persistedState = await settingsPage.isEditorCleanupEnabled();
expect(persistedState).toBe(toggledState);

await settingsPage.toggleEditorCleanup();
const restoredState = await settingsPage.isEditorCleanupEnabled();
expect(restoredState).toBe(initialState);
});

test('should save pipeline commands when changed', async ({
settingsPage,
databasesPage,
}) => {
await settingsPage.expandWorkbench();
await expect(settingsPage.pipelineCommandsValue).toBeVisible();

const initialValue = await settingsPage.getPipelineCommandsValue();
await settingsPage.setPipelineCommandsAndApply(2);
await expect(settingsPage.pipelineCommandsValue).toHaveText('2');

await databasesPage.goto();
await settingsPage.goto();
await settingsPage.expandWorkbench();
const persistedValue = await settingsPage.getPipelineCommandsValue();
expect(persistedValue.trim()).toBe('2');

const restoreValue = initialValue.trim() ? parseInt(initialValue, 10) : 5;
await settingsPage.setPipelineCommandsAndApply(restoreValue);
await expect(settingsPage.pipelineCommandsValue).toHaveText(
String(restoreValue),
);
});
});