Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

test: dashboard max query range limit#6813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ktx-akshay merged 29 commits intomainfromtest--dashboard-max-query-range-limit
Sep 5, 2025

Conversation

@ktx-akshay
Copy link
Collaborator

@ktx-akshayktx-akshay commentedMay 15, 2025•
edited by github-actionsbot
Loading

PR Type

Tests, Enhancement

Description

  • Add dashboard max query range UI test

  • Introduce StreamSettingsPage page object

  • Enhance ManagementPage with streaming toggles

  • Include maxquery test in CI workflow

Diagram Walkthrough

flowchart LR  spec["Add maxquery Playwright spec"] -- uses --> pm["PageManager utilities"]  spec -- updates --> streamPage["StreamSettingsPage (new)"]  spec -- verifies --> uiWarn["Max duration warning visible"]  pm -- ensures --> streamingDisabled["Streaming disabled via ManagementPage"]  ci["GitHub Actions workflow"] -- includes --> spec
Loading

File Walkthrough

Relevant files
Enhancement
streams.js
New StreamSettingsPage for stream max query updates           

tests/ui-testing/pages/dashboardPages/streams.js

  • Add StreamSettingsPage class
  • Implement max query range update flow
  • Add stable locators and waits
  • Handle modal close after save
+57/-0   
managementPage.js
ManagementPage refactor and streaming toggle helpers         

tests/ui-testing/pages/generalPages/managementPage.js

  • Refactor to modern style and quotes
  • Add navigation helpers with validation
  • Add enable/disable streaming utilities
  • Improve selectors and waits
+162/-89
Tests
maxquery.spec.js
Playwright spec for dashboard max query warning                   

tests/ui-testing/playwright-tests/dashboards/maxquery.spec.js

  • Add end-to-end test for max query limit
  • Use PageManager and StreamSettingsPage
  • Validate warning visibility and API response
  • Restore stream setting after test
+107/-0 
Configuration changes
playwright.yml
CI includes max query dashboard test                                         

.github/workflows/playwright.yml

  • Add maxquery.spec.js to Dashboards-Core matrix
  • Minor YAML formatting adjustments
  • Preserve existing test matrices
+75/-70 

greptile-apps[bot] reacted with thumbs up emoji
@ktx-akshayktx-akshayforce-pushed thetest--dashboard-max-query-range-limit branch 3 times, most recently from80b9c93 toc2425b2CompareMay 22, 2025 09:28
@ktx-akshayktx-akshayforce-pushed thetest--dashboard-max-query-range-limit branch 3 times, most recently frome0ff378 to230ad8eCompareJuly 17, 2025 07:27
@ktx-akshayktx-akshayforce-pushed thetest--dashboard-max-query-range-limit branch 2 times, most recently from651be45 to4850684CompareAugust 5, 2025 05:04
@ktx-akshayktx-akshayforce-pushed thetest--dashboard-max-query-range-limit branch fromdafd257 to586e496CompareSeptember 3, 2025 08:51
@ktx-akshayktx-akshay marked this pull request as ready for reviewSeptember 3, 2025 09:42
Copy link
Contributor

@greptile-appsgreptile-appsbot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR adds comprehensive UI test coverage for the dashboard max query range limit functionality. The changes include:

  1. New Test File:maxquery.spec.js is added to validate that dashboards properly display warning messages when query time ranges exceed stream-configured limits (e.g., warning appears for 6-week queries when stream limit is 4 days, and disappears when reduced to 2 hours).

  2. Page Object Pattern Enhancement: A newStreamSettingsPage class is created to encapsulate stream configuration UI interactions, following established testing patterns in the codebase. This provides reusable methods for updating stream max query range settings.

  3. Management Page Extension: TheManagementPage class gains streaming control methods (checkStreaming() andensureStreamingDisabled()) to ensure consistent test environment setup by managing the streaming feature state.

  4. CI Integration: The new test is integrated into the GitHub Actions Playwright workflow under the Dashboards-Core test suite, with YAML formatting standardized across all test configurations.

The implementation follows the existing test architecture using PageManager for UI interactions and includes proper test lifecycle management with setup/teardown procedures. The functionality being tested appears to be a performance safeguard that prevents users from executing resource-intensive queries that exceed configured time limits.

Confidence score: 3/5

  • This PR has moderate risk due to test implementation quality issues that could affect reliability
  • Score reflects concerns about hardcoded timeouts, duplicate code, debugging artifacts, and potential test flakiness from timing dependencies
  • Pay close attention tomaxquery.spec.js for code quality improvements andmanagementPage.js for timeout handling

4 files reviewed, 5 comments

Edit Code Review Bot Settings |Greptile

@github-actions
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Flaky Waits

The method relies on fixed timeouts (e.g., waitForTimeout 3000) instead of waiting for specific network or UI conditions, which can cause flakiness. Prefer waiting on targeted responses or element states already hinted by the commented waitForResponse.

awaitthis.searchInput.click();awaitthis.searchInput.fill(streamName);awaitthis.page.waitForTimeout(3000);// Wait for the expected response// await this.page.waitForResponse(//   (response) =>//     response//       .url()//       .includes(//         "/api/default/streams?type=logs&offset=0&limit=20&keyword=e2e_automate"//       ) && response.status() === 200// );// Wait for stream details to appear and clickawaitthis.streamDetailButton.waitFor({state:"visible",timeout:5000});awaitthis.streamDetailButton.click();
Locator Specificity

Using first() on the 'Stream Detail' button may click the wrong stream if multiple results are present. Consider scoping the locator to the searched stream row to ensure the correct detail modal is opened.

this.streamDetailButton=page.getByRole("button",{name:"Stream Detail"}).first();this.maxQueryInput=page.locator('[data-test="stream-details-max-query-range-input"]');
Save After Toggle

In ensureStreamingDisabled the code clicks the submit button after toggling but does not re-verify the persisted state post-save. Add a post-save check (e.g., reload and assert aria-checked) to ensure the change is committed.

// If Streaming is enabled, click to disable itif(isChecked){console.log("Disabling Streaming...");awaitthis.page.click(toggleSelector);awaitthis.page.waitForTimeout(500);// allow UI to update// Verify toggle is now disabledconstnewCheckedState=awaitthis.page.$eval(toggleSelector,(toggle)=>{returntoggle.getAttribute("aria-checked")==="true";});awaitthis.page.locator('[data-test="dashboard-add-submit"]').click();awaitthis.page.waitForTimeout(5000);if(!newCheckedState){console.log("Streaming has been successfully disabled.");}else{console.log("Failed to disable Streaming.");}}else{console.log("Streaming is already disabled.");}}

@github-actions
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                   Impact
Possible issue
Use locator getAttribute for stability

Accessing attributes with$eval can throw if the element re-renders; use a locator
andgetAttribute to reduce detached element errors. This improves stability under UI
updates.

tests/ui-testing/pages/generalPages/managementPage.js [86-88]

-const isChecked = await this.page.$eval(toggleSelector, (toggle) => {-  return toggle.getAttribute("aria-checked") === "true";-});+const toggle = this.page.locator(toggleSelector);+await toggle.waitFor({ state: "visible" });+const isChecked = (await toggle.getAttribute("aria-checked")) === "true";
Suggestion importance[1-10]: 7

__

Why: Switching from $eval to locator.getAttribute reduces detached element errors during re-renders and matches Playwright best practices. It's contextually correct and enhances stability without changing behavior.

Medium
Avoid fixed sleeps with conditional wait

Replace the fixed timeout with a condition-based wait to avoid flakiness and race
conditions. Wait for the specificstreamName row/button to appear before proceeding.

tests/ui-testing/pages/dashboardPages/streams.js [23-25]

 await this.searchInput.click(); await this.searchInput.fill(streamName);-await this.page.waitForTimeout(3000);+await this.streamDetailButton.filter({ hasText: "Stream Detail" }).waitFor({ state: "visible", timeout: 15000 });
Suggestion importance[1-10]: 6

__

Why: Replacing a hardcoded timeout with a condition-based wait reduces flakiness and aligns with best practices. However, usingthis.streamDetailButton.filter({ hasText: "Stream Detail" }) is redundant since the locator already targets that button, so impact is moderate.

Low
General
Guarantee input is cleared before fill

Ensure the input is truly cleared before typing to prevent concatenated values
causing wrong settings. Use a clear action or select-all-and-delete before filling.

tests/ui-testing/pages/dashboardPages/streams.js [45]

+await this.maxQueryInput.click();+await this.maxQueryInput.press('Control+A');+await this.maxQueryInput.press('Delete'); await this.maxQueryInput.fill(newValue);
Suggestion importance[1-10]: 7

__

Why: Ensuring the input is cleared before filling prevents concatenated values and potential test instability. The approach is accurate and improves reliability with minimal risk.

Medium

@ktx-akshayktx-akshayforce-pushed thetest--dashboard-max-query-range-limit branch 4 times, most recently from38511d7 tob33be23CompareSeptember 4, 2025 12:58
… with enhanced logging and streamlined methods
@ktx-akshayktx-akshayforce-pushed thetest--dashboard-max-query-range-limit branch fromd424afc to2a243c9CompareSeptember 5, 2025 05:05
@ktx-akshayktx-akshay merged commit62d5f2d intomainSep 5, 2025
34 of 40 checks passed
@ktx-akshayktx-akshay deleted the test--dashboard-max-query-range-limit branchSeptember 5, 2025 06:52
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ktx-kirtanktx-kirtanktx-kirtan approved these changes

+1 more reviewer

@greptile-appsgreptile-apps[bot]greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ktx-akshay@ktx-kirtan
[8]ページ先頭
©2009-2025 Movatter.jp