chore(test): include check for correct installer file in update test #13247
Conversation
Signed-off-by: Ondrej Dockal <odockal@redhat.com>
📝 WalkthroughWalkthroughA new Playwright test case was added to the Podman Desktop test suite to verify that the correct installer file is downloaded during the update process, based on the system architecture. The test is designed to run on Windows and Mac platforms, skipping Linux. It determines the expected installer file format and naming pattern according to the platform and architecture, locates the installer in the appropriate directory, and asserts that exactly one installer file matching the expected pattern exists. Additional imports for file system and path operations were included to support these checks. Possibly related issues
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
tests/playwright/src/special-specs/installation/update-install.spec.tsOops! Something went wrong! :( ESLint: 9.31.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'svelte-eslint-parser' imported from /eslint.config.mjs 📜 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). (13)
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Hey @odockal - I've reviewed your changes - here's some feedback:
- Move the test.skip condition out of the test body (e.g. call test.skip(...) before defining the test) so that the callback is never invoked on unsupported platforms.
- Replace the synchronous fs.readdirSync call with async fs.promises.readdir (or add retry/wait logic) to reduce the chance of flakiness when the file isn’t immediately available.
- Anchor your installer filename regex with ^ and $ (and escape special chars) so it only matches the exact expected file and not any similarly named files.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Move the test.skip condition out of the test body (e.g. call test.skip(...) before defining the test) so that the callback is never invoked on unsupported platforms.
- Replace the synchronous fs.readdirSync call with async fs.promises.readdir (or add retry/wait logic) to reduce the chance of flakiness when the file isn’t immediately available.
- Anchor your installer filename regex with ^ and $ (and escape special chars) so it only matches the exact expected file and not any similarly named files.
## Individual Comments
### Comment 1
<location> `tests/playwright/src/special-specs/installation/update-install.spec.ts:159` </location>
<code_context>
+ ? (process.env['LOCALAPPDATA'] ?? path.join(homedir(), 'AppData', 'Local'))
+ : path.join(homedir(), 'Library', 'Caches');
+ const installerPath = path.join(cacheDir, 'podman-desktop-updater', 'pending');
+ const findFiles = fs.readdirSync(installerPath).filter(file => new RegExp(`.*(${fileFormatRegexp})$`).exec(file));
+ playExpect(
+ findFiles.length,
</code_context>
<issue_to_address>
Consider using a stricter regular expression for file matching.
Anchoring the regex to the expected filename pattern (e.g., starting with 'podman-desktop') will help prevent matching unrelated files with the same extension.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
const findFiles = fs.readdirSync(installerPath).filter(file => new RegExp(`.*(${fileFormatRegexp})$`).exec(file));
=======
const findFiles = fs.readdirSync(installerPath).filter(file =>
new RegExp(`^podman-desktop.*\\.(${fileFormatRegexp})$`).exec(file)
);
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ? (process.env['LOCALAPPDATA'] ?? path.join(homedir(), 'AppData', 'Local')) | ||
| : path.join(homedir(), 'Library', 'Caches'); | ||
| const installerPath = path.join(cacheDir, 'podman-desktop-updater', 'pending'); | ||
| const findFiles = fs.readdirSync(installerPath).filter(file => new RegExp(`.*(${fileFormatRegexp})$`).exec(file)); |
There was a problem hiding this comment.
suggestion (bug_risk): Consider using a stricter regular expression for file matching.
Anchoring the regex to the expected filename pattern (e.g., starting with 'podman-desktop') will help prevent matching unrelated files with the same extension.
| const findFiles = fs.readdirSync(installerPath).filter(file => new RegExp(`.*(${fileFormatRegexp})$`).exec(file)); | |
| const findFiles = fs.readdirSync(installerPath).filter(file => | |
| new RegExp(`^podman-desktop.*\\.(${fileFormatRegexp})$`).exec(file) | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/playwright/src/special-specs/installation/update-install.spec.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/color-registry-inject.spec.ts:33-35
Timestamp: 2025-07-02T08:26:44.201Z
Learning: In test files for Podman Desktop, benoitf prefers partial mocking over complete mocking. Partial mocks that only implement the methods actually used in the test are sufficient and preferred over creating complete mock objects.
Learnt from: odockal
PR: podman-desktop/podman-desktop#13022
File: tests/playwright/src/utility/auth-utils.ts:56-82
Timestamp: 2025-06-30T15:28:11.291Z
Learning: Playwright's page.screenshot() method automatically creates directories if they don't exist, so manual directory creation is not needed before taking screenshots in Playwright tests.
tests/playwright/src/special-specs/installation/update-install.spec.ts (2)
Learnt from: dgolovin
PR: podman-desktop/podman-desktop#13030
File: packages/main/src/plugin/cli-tool-registry.ts:108-113
Timestamp: 2025-07-07T17:23:41.302Z
Learning: CLI tool extensions have two distinct `selectVersion` method definitions: one in the installer (which accepts an optional `latest` parameter) and one in the updater (which has different selection logic). The installer's `selectVersion` method signature is `async (latest?: boolean) => Promise<string>` while the updater's is `async () => Promise<string>`.
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13232
File: packages/api/src/repository-infos.ts:21-21
Timestamp: 2025-07-16T15:41:18.035Z
Learning: In the Podman Desktop project, the `with { type: 'json' }` syntax is the correct modern approach for JSON imports with TypeScript 5.8.3 and Vite, not the older `assert { type: 'json' }` syntax.
🔇 Additional comments (5)
tests/playwright/src/special-specs/installation/update-install.spec.ts (5)
19-21: LGTM! Modern Node.js imports are correctly used.The new imports use the recommended
node:prefix syntax and are all necessary for the new test functionality.
31-31: LGTM! Platform utility import correctly extended.The addition of
isWindowsto the existing import is consistent with the codebase pattern and necessary for the new test.
159-159: LGTM! Regular expression patterns are correctly constructed.The regex patterns properly match the expected installer file formats and naming conventions for both platforms and architectures.
Also applies to: 168-168, 172-172
155-158: LGTM! Path construction handles platform differences correctly.The cache directory path construction properly handles platform-specific locations with appropriate fallbacks and uses safe path joining methods.
160-164: LGTM! Test assertions are comprehensive and provide clear error messages.The assertions properly validate both the presence and uniqueness of installer files, with architecture-specific pattern matching and appropriate error handling for unsupported architectures.
Also applies to: 167-176
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Signed-off-by: Ondrej Dockal <odockal@redhat.com>
What does this PR do?
Add a check for downloaded file that happens during updating of Podman Desktop. If will check that file has correct arch or name containing both installers (universal and setup).
podman-desktop-.*-(arch|setup).exeon windowspodman-desktop-.*-(arch|universal).(zip|dmg)on Mac os.Screenshot / video of UI
What issues does this PR fix or reference?
#13246
How to test this PR?
Check the last test in mac/win-update-e2e-test workflows.