HP-2826: Fix failing manager-model-export.spec.ts:4:1 in pw-hiqdev-stock CI due timeout problem #330
Conversation
…ock CI due timeout problem
WalkthroughRefactored download handling in the Playwright test page: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/playwright/page/Index.ts (3)
5-5: Consolidate imports from the same module.The
Downloadtype is imported separately, but line 1 already imports from@playwright/test. Consolidate these imports into a single statement.Apply this diff:
-import { expect, Page } from "@playwright/test"; +import { expect, Page, Download } from "@playwright/test"; import * as fs from "fs"; import AdvancedSearch from "@hipanel-core/helper/AdvancedSearch"; import { Alert, BulkActions, ColumnFilters } from "@hipanel-core/shared/ui/components"; -import {Download} from "@playwright/test";
238-240: Consider verifying the runtime directory exists.The implementation is straightforward. However, if the
runtimedirectory doesn't exist, the subsequentsaveAscall might fail. Consider verifying or creating the directory if this could be an issue in your test environment.
257-266: LGTM!The assertion correctly verifies the downloaded file is not empty and logs the file size for debugging. This is helpful for diagnosing issues in CI environments.
Optional improvement: Consider adding cleanup logic to remove downloaded test files from the
runtimedirectory to prevent accumulation over multiple test runs. This could be done in a teardown method or after the assertions complete.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/playwright/page/Index.ts(2 hunks)
🔇 Additional comments (4)
tests/playwright/page/Index.ts (4)
226-236: LGTM!The implementation correctly uses Playwright's download event handling. The
highlight()call before clicking may help with timing issues in CI environments, andPromise.allensures the download event listener is registered before the click action.
242-246: LGTM!The implementation correctly saves the download and logs the temporary path for debugging purposes. The logging will be helpful for diagnosing download issues in CI environments.
248-255: LGTM!The assertion correctly verifies file existence with a clear error message that includes the expected path. This will help diagnose download failures in CI environments.
216-224: Verify how this refactoring addresses the timeout issue in the PR context.The refactoring improves code organization by separating concerns into distinct helper methods. The sequential flow with explicit awaits ensures each step completes before proceeding—notably, the
awaitonsaveDownloadbefore assertions could address timing issues. However, without access to the original implementation or timeout-related configurations, the claim that this refactoring fixes the timeout issue cannot be confirmed. Request the PR author to clarify:
- What was the original timeout/race condition issue?
- Does this refactoring prevent it, and if so, how?
- Are explicit timeout values needed in any operations?
…sure it will work stable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/playwright/page/Index.ts (2)
216-245: Refactored download flow is clearer; consider making the return type explicitThe split into
checkDownloadByLinkName,triggerDownloadByLinkName,buildDownloadPath, andsaveDownloadmakes the flow much easier to follow and reuse. One small win for maintainability is to declare the return type oftriggerDownloadByLinkNameexplicitly so it’s obvious it returns aDownload:- private async triggerDownloadByLinkName(linkName: string) { + private async triggerDownloadByLinkName(linkName: string): Promise<Download> {This tightens type guarantees if the implementation ever changes and makes the intent clearer at the call site.
Please verify this change against your TypeScript config (e.g.,
tsc --noEmit) to ensure the inferred and explicit types line up as expected.
247-280: Polling helper is reasonable; timeout may be short for slow CI exportsThe
waitForFile/isFileReady/delaytrio is a pragmatic way to de-flake exports by waiting for a non‑empty file. Given the PR is about CI timeouts, the hard‑codedtimeoutMs = 5000might still be tight on slower runners or large exports. You could either increase the default (e.g. 20–30s) or make it configurable from the caller:- private async waitForFile(filePath: string, timeoutMs = 5000): Promise<void> { + private async waitForFile(filePath: string, timeoutMs = 30000): Promise<void> {or
- private async checkDownloadByLinkName(linkName: string): Promise<void> { + private async checkDownloadByLinkName(linkName: string, timeoutMs = 30000): Promise<void> { @@ - await this.waitForFile(downloadPath); + await this.waitForFile(downloadPath, timeoutMs);so tests depending on heavier exports can opt into a higher ceiling without changing the helper logic.
Please run the affected Playwright test(s) on your slowest CI environment to confirm the chosen timeout value fully eliminates the previous flakiness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/playwright/page/Index.ts(2 hunks)
🔇 Additional comments (1)
tests/playwright/page/Index.ts (1)
5-5: Download type import looks goodThe added
Downloadimport matches its usage in the new helpers and keeps the download flow strongly typed; nothing to change here.Please make sure your TypeScript build / Playwright test run passes so that any type- or import-level issues around
Downloadare caught early in CI.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/playwright/page/Index.ts (3)
5-5: Consolidate the import with line 1.The
Downloadtype import can be merged with the existing import from@playwright/teston line 1.Apply this diff:
-import { expect, Page } from "@playwright/test"; +import { expect, Page, Download } from "@playwright/test"; import * as fs from "fs"; import AdvancedSearch from "@hipanel-core/helper/AdvancedSearch"; import { Alert, BulkActions, ColumnFilters } from "@hipanel-core/shared/ui/components"; -import {Download} from "@playwright/test";
225-235: Add explicit return type annotation.The method logic is correct. Consider adding an explicit return type for better type safety and documentation.
Apply this diff:
- private async triggerDownloadByLinkName(linkName: string) { + private async triggerDownloadByLinkName(linkName: string): Promise<Download> { const downloadPromise = this.page.waitForEvent('download');
241-245: Consider using a test logger for better debugging.The logic is correct. The
console.logis acceptable for debugging, but consider using Playwright's test logging or a structured logger for better integration with test reports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/playwright/page/Index.ts(2 hunks)
🔇 Additional comments (5)
tests/playwright/page/Index.ts (5)
216-223: LGTM! Clear refactoring improves maintainability.The method now has a clear flow with well-separated concerns. The explicit
Promise<void>return type and delegation to helper methods improve readability and testability.
247-261: Verify the timeout is sufficient for CI environments.The polling mechanism correctly addresses the race condition in download handling. However, the 5-second default timeout might be insufficient in slow CI environments, especially for large exports.
Consider increasing the default timeout to 10-15 seconds for CI reliability, or make it configurable via environment variable:
- private async waitForFile(filePath: string, timeoutMs = 5000): Promise<void> { + private async waitForFile(filePath: string, timeoutMs = 15000): Promise<void> { const pollingIntervalMs = 100; const deadline = Date.now() + timeoutMs;Alternatively, make it configurable:
private async waitForFile(filePath: string, timeoutMs = parseInt(process.env.DOWNLOAD_TIMEOUT_MS || '10000')): Promise<void> {Monitor the CI logs to determine if downloads complete within 5 seconds consistently.
263-276: LGTM! Robust file validation logic.The check for both existence and non-zero size prevents race conditions where the file is created but not yet written. The logging provides useful debugging information.
278-280: LGTM! Standard delay implementation.The promise-based delay is correctly implemented for the polling mechanism.
237-239: Verify the runtime directory exists before saving files.The
runtime/directory must exist in your test environment, especially in CI. Playwright'sDownload.saveAs()may fail if the target directory doesn't exist. Ensure this directory is created during test setup or before calling this method.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.