Fix tests #477
Fix tests #477
Conversation
|
""" WalkthroughThe updates introduce Playwright test sharding and report merging in the CI workflow, enhance Playwright configuration for multi-reporter output, and refactor E2E test constants and data retrieval for improved consistency. Several utility and page object methods are added or refined, and DOM element identification is improved. Test suites are updated to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant CI
participant Playwright Shard 1
participant Playwright Shard 2
participant Merge Reports
CI->>Playwright Shard 1: Run tests (shard 1)
CI->>Playwright Shard 2: Run tests (shard 2)
Playwright Shard 1->>CI: Upload shard 1 report/results
Playwright Shard 2->>CI: Upload shard 2 report/results
CI->>Merge Reports: Trigger merge job after shards complete
Merge Reports->>CI: Download all shard reports
Merge Reports->>Merge Reports: Merge reports into combined HTML/JSON
Merge Reports->>CI: Upload merged report artifact
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
![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 (4)
e2e/logic/utils.ts (1)
22-24: Consider improving TypeScript typing.The function logic is correct, but the return type
anycould be more specific for better type safety.Consider using a generic type or a more specific return type:
-export function findFirstNodeWithSrc(nodes: { src?: string }[]): any { +export function findFirstNodeWithSrc<T extends { src?: string }>(nodes: T[]): T | undefined { return nodes.find((node) => node.src !== undefined); }e2e/tests/nodeDetailsPanel.spec.ts (1)
5-5: Minor formatting issue with trailing comma.The trailing comma after
GRAPHRAG_SDKin the import statement is unnecessary and should be removed for cleaner code.-import { FLASK_GRAPH, GRAPHRAG_SDK, } from "../config/constants"; +import { FLASK_GRAPH, GRAPHRAG_SDK } from "../config/constants";.github/workflows/playwright.yml (1)
59-59: Fix YAML formatting issues.The static analysis tools identified several formatting issues that should be addressed for code quality.
steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v4Also remove trailing spaces from lines 79, 82, and 85:
- # Create combined results directory + # Create combined results directory - + - # Move merged report to combined directory + # Move merged report to combined directoryAlso applies to: 79-79, 82-82, 85-85
e2e/logic/POM/codeGraph.ts (1)
578-623: Innovative canvas animation detection but consider performance impactThe
waitForCanvasAnimationToEndmethod uses pixel-level comparison to detect animation completion, which is innovative. However, there are some considerations:
- Performance Impact: Comparing entire canvas pixel data using
JSON.stringifyis expensive- Memory Usage:
getImageDatareturns large arrays that are being stringified- Timeout Handling: The method resolves
trueon timeout, which might mask real issuesConsider optimizing the pixel comparison:
- if (JSON.stringify(previousData) === JSON.stringify(currentData)) { + // Compare only a sample of pixels for better performance + let pixelsMatch = true; + for (let i = 0; i < currentData.length; i += 1000) { + if (previousData[i] !== currentData[i]) { + pixelsMatch = false; + break; + } + } + if (pixelsMatch) {Or use a hash-based comparison for better performance:
+ const hash = (data: Uint8ClampedArray) => { + let hash = 0; + for (let i = 0; i < data.length; i += 100) { + hash = ((hash << 5) - hash + data[i]) & 0xffffffff; + } + return hash; + }; + - if (JSON.stringify(previousData) === JSON.stringify(currentData)) { + if (hash(previousData) === hash(currentData)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/playwright.yml(2 hunks)app/components/elementMenu.tsx(1 hunks)e2e/config/constants.ts(1 hunks)e2e/config/testData.ts(1 hunks)e2e/logic/POM/codeGraph.ts(6 hunks)e2e/logic/utils.ts(1 hunks)e2e/tests/canvas.spec.ts(8 hunks)e2e/tests/chat.spec.ts(6 hunks)e2e/tests/nodeDetailsPanel.spec.ts(4 hunks)e2e/tests/searchBar.spec.ts(6 hunks)playwright.config.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
e2e/tests/searchBar.spec.ts (2)
e2e/config/constants.ts (1)
GRAPHRAG_SDK(1-1)e2e/logic/api/apiCalls.ts (1)
ApiCalls(5-42)
e2e/tests/chat.spec.ts (5)
e2e/config/constants.ts (3)
GRAPHRAG_SDK(1-1)CHAT_OPTTIONS_COUNT(3-3)Node_Question(4-4)e2e/logic/POM/codeGraph.ts (1)
CodeGraph(11-625)e2e/logic/utils.ts (1)
delay(3-3)e2e/config/testData.ts (1)
nodesPath(17-20)e2e/logic/api/apiCalls.ts (1)
ApiCalls(5-42)
e2e/logic/POM/codeGraph.ts (1)
e2e/logic/utils.ts (1)
delay(3-3)
🪛 YAMLlint (1.37.1)
.github/workflows/playwright.yml
[warning] 59-59: wrong indentation: expected 6 but found 4
(indentation)
[error] 79-79: trailing spaces
(trailing-spaces)
[error] 82-82: trailing spaces
(trailing-spaces)
[error] 85-85: trailing spaces
(trailing-spaces)
🔇 Additional comments (22)
app/components/elementMenu.tsx (1)
44-44: Good addition for test automation!Adding a stable DOM identifier improves test reliability by providing a consistent selector for the element menu container.
e2e/config/constants.ts (1)
1-2: Good refactoring to unify graph identifiers.Consolidating
GRAPH_IDandPROJECT_NAMEintoGRAPHRAG_SDKand addingFLASK_GRAPHimproves consistency and supports multiple graph contexts.e2e/config/testData.ts (1)
4-4: Confirm the test data update reflects actual autocomplete behaviorI didn’t find any references to “lower” or “lower_items” in the autocomplete implementation, so please manually verify that for the input
"low", the system now suggests"lower_items"as expected.e2e/tests/searchBar.spec.ts (3)
5-5: Good import update for the constants refactoring.Properly updated to use the new unified
GRAPHRAG_SDKconstant.
24-24: Consistent usage of the new constant.All
selectGraph()calls have been properly updated to useGRAPHRAG_SDKinstead of the previous constants.Also applies to: 37-37, 49-49, 59-59, 69-69
73-73: API call updated consistently.The
searchAutoComplete()API call correctly uses the newGRAPHRAG_SDKconstant.playwright.config.ts (1)
25-30: LGTM! Configuration properly supports CI test sharding.The conditional reporter configuration correctly provides JSON output for CI report merging while maintaining simple HTML reporting for local development. This aligns well with the new workflow sharding strategy.
e2e/tests/chat.spec.ts (3)
6-6: LGTM! Constants consolidation improves maintainability.The import update to use centralized constants (
GRAPHRAG_SDK,Node_Question) from the constants file improves code maintainability and consistency across test files.
34-44: Good optimization of test timing and reliability.The reduction from 5 to 3 iterations and addition of 3000ms delays improves test reliability by allowing proper time for asynchronous processing. This should reduce flaky test failures.
123-125: Efficient test optimization by moving API call.Moving the API call before page creation and graph selection is a good optimization that reduces test execution time while maintaining the same validation logic.
e2e/tests/nodeDetailsPanel.spec.ts (2)
24-28: Excellent improvement to test reliability.The addition of
browser.setPageToFullScreen()and the switch togetGraphNodes()with utility functions for node finding should significantly improve test reliability and maintainability.
66-67: Good use of utility function for robust node selection.Using
findFirstNodeWithSrc()instead of hardcoded node selection makes the test more robust and adaptable to different graph structures.e2e/tests/canvas.spec.ts (3)
5-5: LGTM! Constants consolidation improves maintainability.The switch to using
GRAPHRAG_SDKfrom centralized constants improves consistency across the test suite and makes maintenance easier.
46-46: Good addition for canvas synchronization.Adding the
clickCenter()call after graph selection should help ensure consistent canvas state before running assertions, improving test reliability.
53-54: Reasonable tolerance adjustment for flaky tests.Relaxing the assertion tolerance from 0.1 to 0.2 for scale comparisons should reduce test flakiness while still validating the centering functionality effectively.
.github/workflows/playwright.yml (2)
11-14: LGTM! Good implementation of test sharding.The matrix strategy with 2 shards and fail-fast disabled should improve CI performance while ensuring all tests run even if some fail.
54-96: Excellent implementation of report merging.The merge-reports job properly combines shard results into unified reports. The dependency on the test job and artifact handling look correct.
e2e/logic/POM/codeGraph.ts (5)
123-125: LGTM: Well-defined locator for loading indicatorThe new
waitingForResponseImagelocator provides a clean way to target the loading indicator across the entire page, which is an improvement over the more specificpreviousQuestionLoadingImagelocator.
287-295: Good improvement: Dynamic waiting replaces fixed delayThe enhancement to wait for the loading indicator to disappear before retrieving text is a significant improvement over fixed delays. This makes the test more reliable and faster.
196-199: LGTM: Stable element menu locatorUsing an ID selector for the element menu is a good practice for test stability, as IDs are typically more stable than other selectors.
431-444: Excellent enhancement: Robust node clicking with retry logicThe enhanced
nodeClickmethod significantly improves test reliability by:
- Waiting for canvas animations to complete
- Implementing retry logic with hover and click attempts
- Providing clear error messaging when attempts fail
566-576: Confirmed correct usage ofgetGraphDetails()in tests
- Reviewed e2e/tests/canvas.spec.ts: every final-graph assertion now calls
getGraphDetails()and no legacy calls to an older graph-fetch method remain.- The only remaining use of
getGraphNodes()is for initial-state validation, preserving single-responsibility.All tests are synchronized with the updated method—no further changes required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/logic/utils.ts (1)
22-28: LGTM! Consider type safety improvements.The new utility functions are well-implemented and serve clear purposes. Consider these improvements:
- Apply optional chaining as suggested by static analysis:
export function findNodeWithSpecificSrc(nodes: { src?: string }[], srcContent: string): any { - return nodes.find((node) => node.src && node.src.includes(srcContent)); + return nodes.find((node) => node.src?.includes(srcContent)); }
- Improve type safety by using more specific return types:
-export function findFirstNodeWithSrc(nodes: { src?: string }[]): any { +export function findFirstNodeWithSrc(nodes: { src?: string }[]): { src?: string } | undefined { return nodes.find((node) => node.src !== undefined); } -export function findNodeWithSpecificSrc(nodes: { src?: string }[], srcContent: string): any { +export function findNodeWithSpecificSrc(nodes: { src?: string }[], srcContent: string): { src?: string } | undefined { return nodes.find((node) => node.src?.includes(srcContent)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/logic/utils.ts(1 hunks)e2e/tests/nodeDetailsPanel.spec.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Naseem77
PR: FalkorDB/code-graph#477
File: .github/workflows/playwright.yml:40-40
Timestamp: 2025-07-08T16:44:48.096Z
Learning: In Playwright, CLI --reporter flags completely override the reporter configuration in playwright.config.ts, even when the config file has conditional reporter settings for CI environments.
🧬 Code Graph Analysis (1)
e2e/tests/nodeDetailsPanel.spec.ts (5)
e2e/config/constants.ts (2)
GRAPHRAG_SDK(1-1)FLASK_GRAPH(2-2)e2e/logic/utils.ts (2)
findNodeByName(18-20)findNodeWithSpecificSrc(26-28)e2e/logic/api/apiCalls.ts (1)
ApiCalls(5-42)e2e/logic/POM/codeGraph.ts (1)
CodeGraph(11-625)e2e/config/testData.ts (1)
nodes(22-27)
🪛 Biome (1.9.4)
e2e/tests/nodeDetailsPanel.spec.ts
[error] 61-61: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
e2e/logic/utils.ts
[error] 27-27: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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). (2)
- GitHub Check: test (1)
- GitHub Check: test (2)
🔇 Additional comments (4)
e2e/tests/nodeDetailsPanel.spec.ts (4)
61-78: Excellent refactoring of copy functionality test.The test has been improved significantly:
- Uses
FLASK_GRAPHfor a more targeted test scenario- Leverages
findNodeWithSpecificSrcto find a node with specific source content- Properly validates clipboard content against API response
- The 2-second delay on line 71 helps with test stability
This aligns well with the PR objectives of improving test reliability and targeting specific test scenarios.
24-24: Great addition of setPageToFullScreen() for consistency.Adding
await browser.setPageToFullScreen()to all tests ensures consistent UI layout and improves test reliability, which aligns perfectly with the PR objectives.Also applies to: 37-37, 51-51, 66-66, 83-83
25-25: Improved graph selection with specific constants.Using distinct graph constants (
GRAPHRAG_SDKandFLASK_GRAPH) instead of generic identifiers makes the tests more explicit and maintainable.Also applies to: 38-38, 52-52, 67-67, 84-84
26-26: Good simplification with getGraphNodes() method.The change from
getGraphDetails()togetGraphNodes()simplifies coordinate handling and makes the code more focused on the actual node data needed for testing.Also applies to: 39-39, 53-53, 68-68, 85-85
PR Type
Tests, Enhancement
Description
Implement CI sharding for parallel test execution
Fix test stability with improved waiting mechanisms
Update test constants and data for better reliability
Enhance canvas interaction methods with retry logic
Changes diagram
Changes walkthrough 📝
2 files
Update test constants for different graphsConfigure CI reporters and test output6 files
Update search completion test dataUpdate canvas tests with improved stabilityFix chat tests with better timingUpdate node details panel testsUpdate search bar test constantsAdd ID attribute for test targeting3 files
Enhance canvas interactions and waiting mechanismsAdd utility function for finding nodesImplement test sharding and report mergingSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores