test: add dashboard variable testcases #8767
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
PR Reviewer Guide 🔍(Review updated until commit adda1c2)Here are some key observations to aid the review process:
|
Contributor
PR Code Suggestions ✨Latest suggestions up to adda1c2
Previous suggestionsSuggestions up to commit fe0fbdb
|
ktx-akshay
changed the title
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 1 test failed | 280 | 245 | 1 | 19 | 15 | 88% | 16m 57s |
Test Failure Analysis
- dashboard-streaming.spec.js: Timeout errors due to element visibility issues
- dashboard streaming testcases should add dashboard variable with filter configuration: Locator timeout waiting for '[data-test="dashboard-back-btn"]' to be visible.
Root Cause Analysis
- The timeout issue in the test is likely caused by recent changes in the dashboard-variables.js file that may affect element visibility.
Recommended Actions
- Increase the timeout duration in the backToDashboardList method in dashboard-create.js to allow more time for the button to become visible.
- Ensure that the '[data-test="dashboard-back-btn"]' element is present and correctly rendered before the test attempts to interact with it.
- Review the recent changes in dashboard-variables.js to confirm they do not inadvertently affect the visibility of dashboard elements.
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 1 test failed | 155 | 138 | 1 | 11 | 5 | 89% | 47m 13s |
Test Failure Analysis
- alerts-import.spec.js: Assertion failure due to multiple elements matching the expected text
- Alerts Import/Export Destination Import from URL and File: 'Successfully imported' resolved to 2 elements, causing visibility check to fail.
Root Cause Analysis
- The recent changes in dashboard-variables.js and dashboard-streaming.spec.js introduced additional waits that may not directly address the visibility issue in alerts-import.spec.js.
Recommended Actions
- Modify the assertion in alerts-import.spec.js to specify the exact text needed to avoid ambiguity. 2. Review the notification messages to ensure they are unique or adjust the test to handle multiple messages. 3. Increase the timeout for the visibility check if necessary to accommodate slower responses.
Shrinath-O2
changed the title
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 365 | 333 | 0 | 19 | 13 | 91% | 5m 0s |
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 365 | 335 | 0 | 19 | 11 | 92% | 5m 1s |
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 1 test failed | 218 | 201 | 1 | 8 | 8 | 92% | 5m 0s |
Test Failure Analysis
- dashboard.spec.js: Assertion failure due to unexpected canvas count
- dashboard UI testcases should update the line chart correctly when used camel case in custom sql query: Expected canvas count to be greater than 0, but received 0.
Root Cause Analysis
- The changes in dashboard-variables.js may have introduced instability affecting the canvas count.
Recommended Actions
- Investigate the impact of the timeout change in dashboard-variables.js on the canvas rendering. 2. Ensure the custom SQL query returns valid data to populate the line chart correctly. 3. Add logging to verify the canvas count before the assertion in dashboard.spec.js.
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 218 | 199 | 0 | 8 | 11 | 91% | 5m 3s |
ktx-akshay
force-pushed
the
e2e-dashbaord-variable-testcase
branch
from
400aa3a to
ac5a112
Compare
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 1 test failed | 292 | 264 | 1 | 17 | 10 | 90% | 4m 18s |
Test Failure Analysis
- dashboard.spec.js: Assertion failure due to unexpected canvas count
- dashboard UI testcases should update the line chart correctly when used camel case in custom sql query: Expected canvas count to be greater than 0, but received 0.
Root Cause Analysis
- The changes in dashboard-variables.js may have introduced instability affecting the canvas count.
Recommended Actions
- Investigate the impact of the timeout change in dashboard-variables.js on the canvas rendering. 2. Ensure that the custom SQL query returns valid data before asserting on canvas count. 3. Add additional logging to track the canvas count before the assertion in dashboard.spec.js.
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 365 | 326 | 0 | 19 | 20 | 89% | 5m 0s |
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 365 | 336 | 0 | 19 | 10 | 92% | 5m 2s |
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 365 | 336 | 0 | 19 | 10 | 92% | 5m 2s |
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 1 test failed | 203 | 177 | 1 | 14 | 11 | 87% | 4m 40s |
Test Failure Analysis
- dashboard-streaming.spec.js: Timeout issues while waiting for elements to be visible
- dashboard streaming testcases should add dashboard variable with filter configuration: Timeout waiting for option 'ziox' to be visible.
Root Cause Analysis
- The code changes in dashboard-variables.js introduced additional waits for element visibility, which may have contributed to the timeout error.
Recommended Actions
- Increase the timeout duration in dashboard-variables.js from 10000ms to a higher value to accommodate slower loading elements. 2. Review the filter configuration logic to ensure that the expected options are available before attempting to select them. 3. Implement additional logging to capture the state of the page before the timeout occurs for better debugging.
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| 1 test failed | 251 | 231 | 1 | 15 | 4 | 92% | 4m 28s |
Test Failure Analysis
- dashboard-streaming.spec.js: Assertion failure due to unexpected API call count
- dashboard streaming testcases should add dashboard variable with filter configuration: Expected 3 API calls but received 5.
Root Cause Analysis
- The failure is linked to the changes in
dashboard-streaming.spec.jswhere the API call count assertion was not met due to the new waiting method indashboard-variables.js.
Recommended Actions
- Review the API call logic to ensure only 3 calls are made for the test case in
dashboard-streaming.spec.js. - Adjust the expected call count in the assertion to match actual behavior if 5 calls are valid.
- Investigate the implementation of
waitForDependentVariablesToLoadindashboard-variables.jsfor potential logic errors.
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 251 | 232 | 0 | 15 | 4 | 92% | 3m 29s |
Test Run FailedAuthor: Testdino Test Results
Test Failure Analysis
Root Cause Analysis
Recommended Actions
|
ktx-akshay
force-pushed
the
e2e-dashbaord-variable-testcase
branch
from
771752a to
7574f44
Compare
Testdino Test Results
|
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 365 | 334 | 0 | 19 | 12 | 92% | 4m 39s |
Testdino Test Results
|
Test Run FailedAuthor: Testdino Test Results
Test Failure Analysis
Root Cause Analysis
Recommended Actions
|
Test Run FailedAuthor: Testdino Test Results
Test Failure Analysis
Root Cause Analysis
Recommended Actions
|
Testdino Test Results
|
ktx-akshay
force-pushed
the
e2e-dashbaord-variable-testcase
branch
from
a73aef3 to
18a869e
Compare
Testdino Test Results
|
…n the dashboard variable settings.
… wait states and visibility checks
…n dashboard tests
…ashboard variable save actions
…h filter support and enhanced API call tracking
…ing tests with new helper function
…flow for improved clarity and structure
ktx-akshay
force-pushed
the
e2e-dashbaord-variable-testcase
branch
from
18a869e to
adda1c2
Compare
Testdino Test Results
|
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 365 | 342 | 0 | 19 | 4 | 94% | 4m 39s |
Contributor
|
Persistent review updated to latest commit adda1c2 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Greptile Overview
Summary
Adds filter configuration support to dashboard variables, enabling dependent variable relationships where one variable's values can be filtered based on another variable's selection.
Key changes:
- Extended
addDashboardVariablemethod with optionalfilterConfigparameter to support filter name, operator, and value selection - Added
waitForDependentVariablesToLoadhelper method for tracking API calls during dependent variable updates - New comprehensive test case validates the filter configuration flow and verifies dependent variable API calls trigger correctly
- Refactored save/close logic to always execute regardless of
customValueSearchflag
Minor issues:
- Commented-out code blocks should be removed for cleaner codebase
- Test variable extraction loop doesn't use the extracted values
Confidence Score: 4/5
- Safe to merge with minor cleanup recommended
- Functional implementation is solid with proper wait mechanisms and comprehensive test coverage, but contains commented-out code and unused variables that should be cleaned up before merge
- Both files have minor style issues with commented code that should be cleaned up
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| tests/ui-testing/pages/dashboardPages/dashboard-variables.js | 4/5 | Adds optional filter configuration support to dashboard variables with proper waiting mechanisms and new helper method for API call tracking |
| tests/ui-testing/playwright-tests/dashboards/dashboard-streaming.spec.js | 4/5 | New comprehensive test case for dashboard variables with filter configuration and dependent variable API call validation |
Sequence Diagram
sequenceDiagram
participant Test as Test Case
participant PM as PageManager
participant DV as DashboardVariables
participant UI as UI Elements
participant API as Backend API
Test->>PM: Create dashboard with variables
Test->>DV: addDashboardVariable("variablename")
DV->>UI: Configure base variable (no filter)
DV->>UI: Save and close
Test->>DV: addDashboardVariable("variablename12", filterConfig)
DV->>UI: Configure variable with filter
Note over DV,UI: filterName: kubernetes_container_name<br/>operator: =<br/>value: $variablename
DV->>UI: Fill filter configuration
DV->>UI: Save and close
Test->>DV: addDashboardVariable("variablename123", filterConfig)
DV->>UI: Configure dependent variable
Note over DV,UI: filterName: kubernetes_namespace_name<br/>operator: =<br/>value: $variablename12
DV->>UI: Save and close
Test->>DV: selectValueFromVariableDropDown("variablename", "ziox")
DV->>UI: Change base variable value
UI->>API: Request new values for variablename12
UI->>API: Request new values for variablename123
UI->>API: Refresh panel data
API-->>UI: Return filtered results
Test->>Test: Verify >= 3 API calls made
Test->>Test: Assert all responses status 200
Test->>Test: Verify streaming parameters present
2 files reviewed, 4 comments
Comment on lines
+95
to
+98
| // await this.page | ||
| // .locator('[data-test="common-auto-complete-option"]') | ||
| // .getByText(filterConfig.value, { exact: true }) | ||
| // .click(); |
Contributor
There was a problem hiding this comment.
style: commented-out code left in production
Suggested change
| // await this.page | |
| // .locator('[data-test="common-auto-complete-option"]') | |
| // .getByText(filterConfig.value, { exact: true }) | |
| // .click(); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/ui-testing/pages/dashboardPages/dashboard-variables.js
Line: 95:98
Comment:
**style:** commented-out code left in production
```suggestion
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+124
to
+139
| // Save Variable and Close Settings (skip if customValueSearch is true) | ||
| // if (!customValueSearch) { | ||
| const saveBtn = this.page.locator('[data-test="dashboard-variable-save-btn"]'); | ||
| await saveBtn.waitFor({ state: "visible", timeout: 10000 }); | ||
| await saveBtn.click(); | ||
|
|
||
| // Wait for the save action to complete and DOM to stabilize | ||
| // await this.page.waitForTimeout(3000); | ||
| await this.page.waitForLoadState("networkidle"); | ||
|
|
||
| // Use JavaScript evaluation to click the close button to avoid DOM instability issues | ||
| await this.page.evaluate(() => { | ||
| const closeBtn = document.querySelector('[data-test="dashboard-settings-close-btn"]'); | ||
| if (closeBtn) closeBtn.click(); | ||
| }); | ||
| // } |
Contributor
There was a problem hiding this comment.
style: commented-out conditional logic and cleanup note - remove or implement properly
Suggested change
| // Save Variable and Close Settings (skip if customValueSearch is true) | |
| // if (!customValueSearch) { | |
| const saveBtn = this.page.locator('[data-test="dashboard-variable-save-btn"]'); | |
| await saveBtn.waitFor({ state: "visible", timeout: 10000 }); | |
| await saveBtn.click(); | |
| // Wait for the save action to complete and DOM to stabilize | |
| // await this.page.waitForTimeout(3000); | |
| await this.page.waitForLoadState("networkidle"); | |
| // Use JavaScript evaluation to click the close button to avoid DOM instability issues | |
| await this.page.evaluate(() => { | |
| const closeBtn = document.querySelector('[data-test="dashboard-settings-close-btn"]'); | |
| if (closeBtn) closeBtn.click(); | |
| }); | |
| // } | |
| // Save Variable and Close Settings | |
| const saveBtn = this.page.locator('[data-test="dashboard-variable-save-btn"]'); | |
| await saveBtn.waitFor({ state: "visible", timeout: 10000 }); | |
| await saveBtn.click(); | |
| // Wait for the save action to complete and DOM to stabilize | |
| await this.page.waitForLoadState("networkidle"); | |
| // Use JavaScript evaluation to click the close button to avoid DOM instability issues | |
| await this.page.evaluate(() => { | |
| const closeBtn = document.querySelector('[data-test="dashboard-settings-close-btn"]'); | |
| if (closeBtn) closeBtn.click(); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/ui-testing/pages/dashboardPages/dashboard-variables.js
Line: 124:139
Comment:
**style:** commented-out conditional logic and cleanup note - remove or implement properly
```suggestion
// Save Variable and Close Settings
const saveBtn = this.page.locator('[data-test="dashboard-variable-save-btn"]');
await saveBtn.waitFor({ state: "visible", timeout: 10000 });
await saveBtn.click();
// Wait for the save action to complete and DOM to stabilize
await this.page.waitForLoadState("networkidle");
// Use JavaScript evaluation to click the close button to avoid DOM instability issues
await this.page.evaluate(() => {
const closeBtn = document.querySelector('[data-test="dashboard-settings-close-btn"]');
if (closeBtn) closeBtn.click();
});
```
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| ); | ||
|
|
||
| // Add second variable with filter configuration |
Contributor
There was a problem hiding this comment.
style: comment says "Add second variable" but this is actually the third variable - update to "Add third variable with filter configuration"
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/ui-testing/playwright-tests/dashboards/dashboard-streaming.spec.js
Line: 218:218
Comment:
**style:** comment says "Add second variable" but this is actually the third variable - update to "Add third variable with filter configuration"
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+323
to
+327
| const newCalls = valuesResponses.slice(noOfPreviousCalls); | ||
| newCalls.forEach((call, idx) => { | ||
| const urlParts = call.url.split('/'); | ||
| const relevantPart = urlParts[urlParts.length - 1] || urlParts[urlParts.length - 2]; | ||
| }); |
Contributor
There was a problem hiding this comment.
style: variables extracted but never used - remove unused code
Suggested change
| const newCalls = valuesResponses.slice(noOfPreviousCalls); | |
| newCalls.forEach((call, idx) => { | |
| const urlParts = call.url.split('/'); | |
| const relevantPart = urlParts[urlParts.length - 1] || urlParts[urlParts.length - 2]; | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/ui-testing/playwright-tests/dashboards/dashboard-streaming.spec.js
Line: 323:327
Comment:
**style:** variables extracted but never used - remove unused code
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.
uddhavdave
pushed a commit
that referenced
this pull request
Oct 27, 2025
### **PR Type**
Enhancement, Tests
___
### **Description**
- Add filter config to variable creation
- Improve save/close stability with waits
- Add helper to await dependent API calls
- New e2e test for filtered variables
___
### Diagram Walkthrough
```mermaid
flowchart LR
varsJS["dashboard-variables.js"]
testJS["dashboard-streaming.spec.js"]
feature["Filter config for variables"]
stability["Stability and wait improvements"]
helper["Dependent API calls wait helper"]
e2e["E2E test for filtered variables"]
varsJS -- "implement" --> feature
varsJS -- "add" --> stability
varsJS -- "introduce" --> helper
testJS -- "use" --> feature
testJS -- "validate" --> helper
testJS -- "verify streaming behavior" --> e2e
```
<details> <summary><h3> File Walkthrough</h3></summary>
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table>
<tr>
<td>
<details>
<summary><strong>dashboard-variables.js</strong><dd><code>Variable
creation gains filter support and waits</code>
</dd></summary>
<hr>
tests/ui-testing/pages/dashboardPages/dashboard-variables.js
<ul><li>Extend addDashboardVariable with filterConfig param.<br> <li>
Add robust waits and save/close handling.<br> <li> Implement
waitForDependentVariablesToLoad helper.<br> <li> Improve selectors and
option picking with visibility checks.</ul>
</details>
</td>
<td><a
href="https://github.com/openobserve/openobserve/pull/8767/files#diff-67d38d39bb67f4a0cbfa473ee81ad50b1fffebc40c635417e58905f3c865a6e2">+96/-19</a>
</td>
</tr>
</table></td></tr><tr><td><strong>Tests</strong></td><td><table>
<tr>
<td>
<details>
<summary><strong>dashboard-streaming.spec.js</strong><dd><code>Streaming
test validates filtered variable dependencies</code>
</dd></summary>
<hr>
tests/ui-testing/playwright-tests/dashboards/dashboard-streaming.spec.js
<ul><li>Add test for variables with filter chains.<br> <li> Track
streaming values API responses.<br> <li> Assert dependent calls and
statuses.<br> <li> Create panel and verify variable-driven
filtering.</ul>
</details>
</td>
<td><a
href="https://github.com/openobserve/openobserve/pull/8767/files#diff-92f44296937f53cb448371d1c61510c8c910fbecab83b02b8e89490d5832be19">+203/-0</a>
</td>
</tr>
</table></td></tr></tr></tbody></table>
</details>
___
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Type
Enhancement, Tests
Description
Add filter config to variable creation
Improve save/close stability with waits
Add helper to await dependent API calls
New e2e test for filtered variables
Diagram Walkthrough
File Walkthrough
dashboard-variables.js
Variable creation gains filter support and waitstests/ui-testing/pages/dashboardPages/dashboard-variables.js
dashboard-streaming.spec.js
Streaming test validates filtered variable dependenciestests/ui-testing/playwright-tests/dashboards/dashboard-streaming.spec.js