test: feature flags #2302
test: feature flags #2302
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA data-testid attribute was added to the feature flags tab in the settings page for improved test targeting. Additionally, a comprehensive functional test suite was introduced to automate toggling and verifying visibility of multiple feature flags, including X-Rays, Incidents, Tasks, Risk Acceptances, Exceptions, Findings Tracking, Ebios RM, Scoring Assistant, Vulnerabilities, Compliance, Third Party, Privacy, Experimental, and Inherent Risk, ensuring UI elements reflect the correct visibility state and restoring original flag settings afterward. Changes
Estimated code review effort3 (30–60 minutes) Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/tests/functional/detailed/feature-flags.test.ts (2)
14-14: Consider using the data-testid for more reliable element selection.While the text-based selector works, consider using the
data-testid="feature-flags-btn"that was added to the settings page for more reliable element selection.- await page.getByText(/^ Feature flags$/).click(); + await page.getByTestId('feature-flags-btn').click();
41-41: Apply the same improvement for consistency.For consistency with the earlier suggestion, consider using the data-testid here as well.
- await page.getByText(/^ Feature flags$/).click(); + await page.getByTestId('feature-flags-btn').click();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/routes/(app)/(internal)/settings/+page.svelte(1 hunks)frontend/tests/functional/detailed/feature-flags.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/tests/functional/detailed/feature-flags.test.ts (1)
frontend/tests/utils/page-content.ts (1)
PageContent(6-161)
🔇 Additional comments (6)
frontend/src/routes/(app)/(internal)/settings/+page.svelte (1)
22-24: LGTM! Clean addition of test identifier.The
data-testid="feature-flags-btn"attribute is appropriately added to support automated testing. The identifier name is descriptive and follows good naming conventions.frontend/tests/functional/detailed/feature-flags.test.ts (5)
1-4: LGTM! Proper test imports and setup.The imports are correctly structured and follow the established testing patterns in the codebase.
5-14: LGTM! Clear test navigation flow.The test properly sets up authentication and navigates to the settings page. The URL assertion provides good verification of navigation success.
16-26: LGTM! Proper toggle state management.The code correctly retrieves toggle elements and manages their initial state. The conditional checking prevents unnecessary clicks when toggles are already in the desired state.
33-38: LGTM! Proper verification of feature flag effects.The test correctly verifies that the X-Rays text is not visible on the Operations page and the Inherent Level text is not visible on the Risk Scenarios page when the respective feature flags are disabled.
39-52: LGTM! Excellent cleanup strategy with try/finally block.The finally block ensures that the feature flags are restored to their original state regardless of test success or failure. This is crucial for test isolation and prevents side effects on subsequent tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/tests/functional/detailed/feature-flags.test.ts (1)
1-251: Consider reducing code duplication with a test factory pattern.While each individual test is correct, there's significant code duplication across all tests. Consider implementing a test factory pattern to reduce maintenance burden and improve consistency.
Example refactor approach:
// Test configuration object const featureFlagTests = [ { name: 'X-Rays', flagId: 'xrays', navigation: () => page.getByText('Operations').click(), element: () => page.getByText('X-Rays', { exact: false }) }, { name: 'Incidents', flagId: 'incidents', navigation: () => page.getByText('Operations').click(), element: () => page.getByText('Incidents', { exact: false }) } // ... other configurations ]; // Factory function featureFlagTests.forEach(({ name, flagId, navigation, element }) => { test(`Feature Flags - ${name} visibility toggling`, async ({ page }) => { const loginPage = new LoginPage(page); await loginPage.goto(); await loginPage.login(); await navigation(); await expect(element()).toBeVisible(); await toggleFeatureFlag(page, flagId, false); await navigation(); await expect(element()).not.toBeVisible(); await toggleFeatureFlag(page, flagId, true); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/tests/functional/detailed/feature-flags.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/tests/functional/detailed/feature-flags.test.ts (1)
frontend/tests/utils/page-content.ts (1)
PageContent(6-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
frontend/tests/functional/detailed/feature-flags.test.ts (5)
21-35: LGTM! Operations section tests follow consistent pattern.The X-Rays, Incidents, and Tasks tests all follow the same reliable pattern for testing Operations section feature flags. The logic correctly verifies initial visibility, toggles the flag off to confirm hiding, and restores the original state.
Also applies to: 38-52, 55-69
72-86: LGTM! Governance section tests are well-structured.The Risk Acceptances, Exceptions, and Findings Tracking tests properly test governance section feature flags with consistent navigation and assertions.
Also applies to: 89-103, 106-120
123-137: LGTM! Risk section tests cover important functionality.The Ebios RM, Scoring Assistant, and Vulnerabilities tests appropriately verify risk section feature flag behavior. Good use of specific test IDs for accordion items where available.
Also applies to: 140-154, 157-171
174-186: LGTM! Top-level navigation tests are comprehensive.The Compliance, Third Party, Privacy, and Experimental tests properly verify main navigation feature flags. These tests efficiently check accordion items and main navigation elements.
Also applies to: 189-201, 204-216, 219-233
236-251: LGTM! Inherent Risk test demonstrates good use of PageContent utility.This test effectively uses the
PageContentclass from the test utilities, which is appropriate for testing page-specific elements like the "Inherent Level" text on the Risk Scenarios page.
| const toggleFeatureFlag = async (page, flagTestId, enable) => { | ||
| await page.getByTestId('accordion-item-extra').click(); | ||
| await page.getByTestId('accordion-item-settings').click(); | ||
| await expect(page).toHaveURL('/settings'); | ||
| await page.getByText(/^ Feature flags$/).click(); | ||
|
|
||
| const toggle = page.getByTestId(`form-input-${flagTestId}`); | ||
| if ((await toggle.isChecked()) !== enable) { | ||
| await toggle.click(); | ||
| await page.getByRole('button', { name: 'Save' }).click(); | ||
| await page.waitForTimeout(500); | ||
| await page.reload(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and improving navigation reliability.
The helper function has several areas for improvement:
- Missing error handling: No validation if the settings page loads correctly or if the feature flag toggle exists
- Hard-coded navigation: Uses text-based navigation which could be brittle if UI text changes
- Fixed timeout remains: Line 15 still uses
waitForTimeout(500)despite past review comments suggesting more reliable alternatives
-const toggleFeatureFlag = async (page, flagTestId, enable) => {
+const toggleFeatureFlag = async (page, flagTestId, enable) => {
+ // Navigate to settings with better error handling
await page.getByTestId('accordion-item-extra').click();
await page.getByTestId('accordion-item-settings').click();
await expect(page).toHaveURL('/settings');
- await page.getByText(/^ Feature flags$/).click();
+
+ // Use data-testid for more reliable navigation
+ await page.getByTestId('feature-flags-btn').click();
const toggle = page.getByTestId(`form-input-${flagTestId}`);
+ await expect(toggle).toBeVisible();
+
if ((await toggle.isChecked()) !== enable) {
await toggle.click();
await page.getByRole('button', { name: 'Save' }).click();
- await page.waitForTimeout(500);
+ await page.waitForLoadState('networkidle');
await page.reload();
+ await page.waitForLoadState('networkidle');
}
};📝 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.
| const toggleFeatureFlag = async (page, flagTestId, enable) => { | |
| await page.getByTestId('accordion-item-extra').click(); | |
| await page.getByTestId('accordion-item-settings').click(); | |
| await expect(page).toHaveURL('/settings'); | |
| await page.getByText(/^ Feature flags$/).click(); | |
| const toggle = page.getByTestId(`form-input-${flagTestId}`); | |
| if ((await toggle.isChecked()) !== enable) { | |
| await toggle.click(); | |
| await page.getByRole('button', { name: 'Save' }).click(); | |
| await page.waitForTimeout(500); | |
| await page.reload(); | |
| } | |
| }; | |
| const toggleFeatureFlag = async (page, flagTestId, enable) => { | |
| // Navigate to settings with better error handling | |
| await page.getByTestId('accordion-item-extra').click(); | |
| await page.getByTestId('accordion-item-settings').click(); | |
| await expect(page).toHaveURL('/settings'); | |
| // Use data-testid for more reliable navigation | |
| await page.getByTestId('feature-flags-btn').click(); | |
| const toggle = page.getByTestId(`form-input-${flagTestId}`); | |
| await expect(toggle).toBeVisible(); | |
| if ((await toggle.isChecked()) !== enable) { | |
| await toggle.click(); | |
| await page.getByRole('button', { name: 'Save' }).click(); | |
| await page.waitForLoadState('networkidle'); | |
| await page.reload(); | |
| await page.waitForLoadState('networkidle'); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In frontend/tests/functional/detailed/feature-flags.test.ts around lines 5 to
18, the toggleFeatureFlag function lacks error handling for page navigation and
element presence, uses brittle text-based navigation, and relies on a fixed
timeout. To fix this, add checks to confirm the settings page loads successfully
and the toggle element exists before interacting. Replace text-based navigation
with more stable selectors like test IDs or roles. Remove the fixed
waitForTimeout and instead use explicit waits for elements or network idle to
ensure reliable synchronization.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
frontend/tests/functional/detailed/feature-flags.test.ts (2)
5-18: The helper function still has reliability issues despite past feedback.Several issues remain unaddressed from previous reviews:
- Still using
waitForTimeout(500)on line 15 instead of more reliable waiting strategies- Text-based navigation on line 9 remains brittle
- Missing error handling for navigation and element presence
- No validation that the settings page loads correctly
9-9: Replace text-based navigation with data-testid selector.The AI summary mentions that a
data-testid="feature-flags-btn"attribute was added to the settings page. Use this more reliable selector instead of text-based navigation.- await page.getByText(/^ Feature flags$/).click(); + await page.getByTestId('feature-flags-btn').click();
🧹 Nitpick comments (2)
frontend/tests/functional/detailed/feature-flags.test.ts (2)
21-35: Good test structure but could benefit from better assertions.The test follows a clear pattern: verify initial state, toggle flag, verify changed state, restore flag. However, the navigation and assertions could be more robust.
Consider adding explicit waits for navigation completion:
await page.getByText('Operations').click(); + await page.waitForLoadState('networkidle'); await expect(page.getByText('X-Rays', { exact: false })).toBeVisible();
21-408: Consider refactoring to reduce code duplication.The test cases follow very similar patterns with significant code duplication. Consider extracting common functionality into reusable helper functions or parameterized tests.
Example approach:
const testFeatureFlagVisibility = async ( page: Page, flagId: string, navigationAction: () => Promise<void>, elementSelector: string | Locator, testName: string ) => { const loginPage = new LoginPage(page); await loginPage.goto(); await loginPage.login(); await navigationAction(); await expect(elementSelector).toBeVisible(); await toggleFeatureFlag(page, flagId, false); await navigationAction(); await expect(elementSelector).not.toBeVisible(); await toggleFeatureFlag(page, flagId, true); }; // Usage test('Feature Flags - X-Rays visibility toggling', async ({ page }) => { await testFeatureFlagVisibility( page, 'xrays', () => page.getByText('Operations').click(), page.getByText('X-Rays', { exact: false }), 'X-Rays' ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/tests/functional/detailed/feature-flags.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/tests/functional/detailed/feature-flags.test.ts (1)
frontend/tests/utils/page-content.ts (1)
PageContent(6-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
| test('Feature Flags - Inherent Risk visibility toggling on Risk Assessment details', async ({ | ||
| page | ||
| }) => { | ||
| const loginPage = new LoginPage(page); | ||
| await loginPage.goto(); | ||
| await loginPage.login(); | ||
|
|
||
| // Create folder | ||
| await page.getByRole('button', { name: 'Organization' }).click(); | ||
| await page.getByTestId('accordion-item-folders').click(); | ||
| await page.getByTestId('add-button').click(); | ||
| await page.getByTestId('form-input-name').fill('feature-flag-folder'); | ||
| await page.getByTestId('save-button').click(); | ||
|
|
||
| // Create perimeter | ||
| await page.getByRole('button', { name: 'Organization' }).click(); | ||
| await page.getByTestId('accordion-item-perimeters').click(); | ||
| await page.waitForTimeout(200); | ||
| await page.getByTestId('add-button').click(); | ||
| await page.getByTestId('form-input-name').fill('feature-flag-perimeter'); | ||
| await page.getByTestId('form-input-folder').click(); | ||
| await page.getByRole('option', { name: 'feature-flag-folder' }).click(); | ||
| await page.getByTestId('save-button').click(); | ||
|
|
||
| // Create risk assessment | ||
| await page.getByTestId('accordion-item-risk').click(); | ||
| await page.getByTestId('accordion-item-risk-assessments').click(); | ||
| await page.waitForTimeout(200); | ||
| await page.getByTestId('add-button').click(); | ||
| await page.getByTestId('form-input-name').fill('test-risk-assessment'); | ||
| await page.getByTestId('form-input-perimeter').click(); | ||
| await page.getByRole('option', { name: 'feature-flag-folder/feature-flag-perimeter' }).click(); | ||
| await page.getByTestId('save-button').click(); | ||
| await page.waitForTimeout(200); | ||
|
|
||
| // Check Inherent Level visibility | ||
| await page.getByText('test-risk-assessment').click(); | ||
| await expect(page.getByText('Inherent Level', { exact: false })).toBeVisible(); | ||
|
|
||
| await toggleFeatureFlag(page, 'inherent-risk', false); | ||
| await page.getByTestId('accordion-item-risk').click(); | ||
| await page.getByTestId('accordion-item-risk-assessments').click(); | ||
| await page.waitForTimeout(200); | ||
| await page.getByText('test-risk-assessment').click(); | ||
| await expect(page.getByText('Inherent Level', { exact: false })).not.toBeVisible(); | ||
|
|
||
| await toggleFeatureFlag(page, 'inherent-risk', true); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test creates complex setup but lacks proper isolation.
This test creates folders, perimeters, and risk assessments but doesn't clean up after itself, potentially affecting other tests. The cleanup only happens in a separate test at the end of the file.
Consider using test fixtures or beforeEach/afterEach hooks to ensure proper test isolation:
test.describe('Feature Flags - Inherent Risk', () => {
let testFolder: string;
let testPerimeter: string;
test.beforeEach(async ({ page }) => {
// Setup test data
testFolder = `test-folder-${Date.now()}`;
// Create folder and perimeter
});
test.afterEach(async ({ page }) => {
// Cleanup test data
});
});🤖 Prompt for AI Agents
In frontend/tests/functional/detailed/feature-flags.test.ts around lines 254 to
301, the test creates folders, perimeters, and risk assessments but does not
clean up after itself, risking interference with other tests. Refactor the test
by wrapping it in a test.describe block and use beforeEach hooks to set up the
required test data (folders, perimeters) with unique names, and afterEach hooks
to delete or clean up the created data. This ensures proper test isolation and
prevents side effects across tests.
| // Create perimeter | ||
| await page.getByRole('button', { name: 'Organization' }).click(); | ||
| await page.getByTestId('accordion-item-perimeters').click(); | ||
| await page.waitForTimeout(200); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove fixed timeouts in favor of more reliable waiting strategies.
Multiple instances of waitForTimeout(200) and waitForTimeout(500) appear throughout the tests. These create potential flakiness and should be replaced with explicit waits for elements or page states.
- await page.waitForTimeout(200);
+ await page.waitForLoadState('networkidle');Or wait for specific elements to be visible/ready before proceeding.
Also applies to: 281-281, 287-287, 296-296, 338-338, 370-370, 378-378, 400-400
🤖 Prompt for AI Agents
In frontend/tests/functional/detailed/feature-flags.test.ts at line 271, replace
the fixed timeout waitForTimeout(200) with a more reliable waiting strategy such
as waiting explicitly for a specific element to be visible or a particular page
state to be ready. Apply the same approach to the other lines mentioned (281,
287, 296, 338, 370, 378, 400) to improve test stability and reduce flakiness.
…lags-settings-module
…lags-settings-module
Here is a file testing the feature flags by verifying the x rays section disappear when the checkbox if on false state and check if the inherent level is shown on the risk scenario page when activated.
Summary by CodeRabbit
New Features
Tests