use argos for snapshot tests#1716
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughA new Playwright snapshot utility and Argos CI integration were added to the frontend viewer. The Playwright configuration was modularized to manage reporters for local and CI environments. Tests were updated to use the new snapshot utility and simplified viewport handling. The package.json was updated with the Argos Playwright dependency. Changes
Poem
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 (
|
UI unit Tests10 tests 10 ✅ 0s ⏱️ Results for commit d5094ce. ♻️ This comment has been updated with latest results. |
C# Unit Tests116 tests 116 ✅ 10s ⏱️ Results for commit d5094ce. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/viewer/tests/project-view-snapshots.test.ts (1)
4-7: Remove unused viewport configuration.The
viewportsarray is no longer used since viewport handling has been moved to theassertSnapshotutility. This code can be safely removed.-const viewports = [ - {height: 720, width: 1280, name: 'medium'}, - {height: 812, width: 375, name: 'iphone-11'}, -];frontend/viewer/playwright.config.ts (1)
33-37: Remove commented code to improve maintainability.The commented-out old configuration should be removed since the new modular approach is now in place. Keeping old code commented can lead to confusion and maintenance overhead.
- // process.env.CI - // ? [['github'], ['list'], ['junit', {outputFile: 'test-results/results.xml'}]] - // // Putting the HTML report in a subdirectory of the main output directory results in a warning log - // // stating that it will "lead to artifact loss" but the warning in this case is not accurate - // : [['list'], ['html', {outputFolder: 'html-test-results', open: 'never'}]],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-default-view-port-iphone-11-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-default-view-port-medium-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-default-viewport-iphone-11---dark-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-default-viewport-iphone-11---light-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-default-viewport-medium---dark-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-default-viewport-medium---light-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-selected-entry-view-port-iphone-11-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-selected-entry-view-port-medium-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-selected-entry-viewport-iphone-11---dark-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-selected-entry-viewport-iphone-11---light-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-selected-entry-viewport-medium---dark-1-chromium-win32.pngis excluded by!**/*.pngfrontend/viewer/tests/project-view-snapshots.test.ts-snapshots/ui-snapshot-selected-entry-viewport-medium---light-1-chromium-win32.pngis excluded by!**/*.png
📒 Files selected for processing (4)
frontend/viewer/package.json(1 hunks)frontend/viewer/playwright.config.ts(2 hunks)frontend/viewer/tests/project-view-snapshots.test.ts(1 hunks)frontend/viewer/tests/snapshot.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build API / publish-api
- GitHub Check: Build UI / publish-ui
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
frontend/viewer/package.json (1)
37-37:Details
✅ Verification successful
Verify the package version is current and secure.
The addition of the Argos CI Playwright integration looks good. Please verify that version 5.0.4 is the latest available version and check for any security advisories.
🌐 Web query:
What is the latest version of @argos-ci/playwright package and are there any known security vulnerabilities?💡 Result:
Latest Version of @argos-ci/playwright
The latest published version of the
@argos-ci/playwrightpackage is 5.0.4, which was last released on May 15, 2025[2].Security Vulnerabilities
There are currently no publicly reported security vulnerabilities specific to the
@argos-ci/playwrightpackage. No security advisories or vulnerability databases, including npm and Snyk, list any active vulnerabilities for this package as of the latest available information[2][6][10].
- The Playwright core package (maintained separately) had a notable security incident in version 1.45, involving malicious code[7]. However, this does not affect
@argos-ci/playwright, which is a separate integration for visual testing.- Leading security trackers and monitoring tools do not list any unresolved or critical issues related to
@argos-ci/playwrightthrough its most recent release[2][10].Summary Table
Attribute Details Latest Version 5.0.4 Release Date 2025-05-15 Known Vulnerabilities None reported If you are using the latest release (
5.0.4), your installation should be secure, provided you also keep Playwright and other related dependencies up to date[2][6].Citations:
- 1: https://github.com/argos-ci/argos-javascript/releases
- 2: https://www.npmjs.com/package/@argos-ci/playwright
- 3: https://argos-ci.com/changelog
- 4: https://github.com/argos-ci/argos-playwright
- 5: https://playwright.dev/docs/release-notes
- 6: https://argos-ci.com/docs/playwright
- 7: https://vulert.com/vuln-db/npm-playwright-1-45-177865
- 8: https://argos-ci.com/playwright
- 9: https://argo-cd.readthedocs.io/en/stable/security_considerations/
- 10: https://security.snyk.io/package/npm/playwright
Dependency version and security status confirmed
Confirmed that
@argos-ci/playwright@5.0.4(released 2025-05-15) is the latest available version and has no known security vulnerabilities. The devDependency addition infrontend/viewer/package.json(line 37) is approved.frontend/viewer/tests/project-view-snapshots.test.ts (3)
2-2: Update import if snapshot utility is renamed.The import looks correct, but if the
assertSnapshotfunction insnapshot.tsis renamed totakeSnapshot(as suggested in that file's review), this import will need to be updated accordingly.
17-17: Excellent improvement in assertion specificity.Changing from
.not.toBeVisible()to.toHaveCount(0)is a great improvement. This is more explicit and clearly indicates that you're asserting the absence of elements rather than just their visibility state.Also applies to: 25-26, 34-34, 36-36
18-18: Good integration with the new snapshot utility.The replacement of
expect(page).toHaveScreenshot()withassertSnapshot()properly integrates with the Argos CI workflow. The snapshot names are descriptive and follow a good naming convention.Also applies to: 28-28
frontend/viewer/playwright.config.ts (3)
1-1: Good addition of type import for better type safety.Adding the
ReporterDescriptiontype import improves type safety for the reporter configuration arrays.
7-16: Excellent modular approach to reporter configuration.The separation into
allReporters,localReporters, andciReportersarrays creates a clean, maintainable configuration structure. The Argos CI reporter configuration withuploadToArgos: !!process.env.CIis correctly set up to only upload in CI environments.
28-32: Clean conditional reporter setup.The conditional spreading of reporter arrays based on the environment is well-implemented and readable.
| export function assertSnapshot(...args: Parameters<typeof argosScreenshot>): Promise<void> { | ||
| args[2] = { | ||
| viewports: [ | ||
| { height: 720, width: 1280 }, | ||
| 'iphone-x' | ||
| ], | ||
| ...args[2] | ||
| }; | ||
| return argosScreenshot(...args); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to avoid mutating input parameters and improve robustness.
The function has several issues that should be addressed:
- Mutation of input parameters: Directly modifying
args[2]mutates the input, which can cause unexpected behavior. - Unsafe array access: The code assumes
args[2]exists without checking. - Misleading function name:
assertSnapshotsuggests assertion, but it's actually taking a screenshot.
Apply this diff to improve the implementation:
-export function assertSnapshot(...args: Parameters<typeof argosScreenshot>): Promise<void> {
- args[2] = {
- viewports: [
- { height: 720, width: 1280 },
- 'iphone-x'
- ],
- ...args[2]
- };
- return argosScreenshot(...args);
-}
+export function takeSnapshot(
+ page: Parameters<typeof argosScreenshot>[0],
+ name: Parameters<typeof argosScreenshot>[1],
+ options?: Parameters<typeof argosScreenshot>[2]
+): Promise<void> {
+ const defaultOptions = {
+ viewports: [
+ { height: 720, width: 1280 },
+ 'iphone-x'
+ ],
+ ...options
+ };
+ return argosScreenshot(page, name, defaultOptions);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function assertSnapshot(...args: Parameters<typeof argosScreenshot>): Promise<void> { | |
| args[2] = { | |
| viewports: [ | |
| { height: 720, width: 1280 }, | |
| 'iphone-x' | |
| ], | |
| ...args[2] | |
| }; | |
| return argosScreenshot(...args); | |
| } | |
| export function takeSnapshot( | |
| page: Parameters<typeof argosScreenshot>[0], | |
| name: Parameters<typeof argosScreenshot>[1], | |
| options?: Parameters<typeof argosScreenshot>[2] | |
| ): Promise<void> { | |
| const defaultOptions = { | |
| viewports: [ | |
| { height: 720, width: 1280 }, | |
| 'iphone-x' | |
| ], | |
| ...options | |
| }; | |
| return argosScreenshot(page, name, defaultOptions); | |
| } |
🤖 Prompt for AI Agents
In frontend/viewer/tests/snapshot.ts around lines 2 to 11, avoid mutating the
input parameter args by creating a new options object instead of modifying
args[2] directly. First, safely check if args[2] exists and is an object; if
not, initialize it as an empty object. Then, create a new options object that
merges the default viewports with the existing options from args[2]. Finally,
call argosScreenshot with the original arguments but replace the third argument
with this new options object. Also, consider renaming the function to better
reflect its purpose, such as takeScreenshot, to avoid confusion.
There was a problem hiding this comment.
Yeah, I think it would be nice to not use args or Parameters.
There was a problem hiding this comment.
And just for the record, I don't have a better name suggestion than "assertSnapshot" 😆. Although..."assertScreenshot" does seem a tad better. "snapshot" is not very specific.
There was a problem hiding this comment.
@myieye Perfect! I'll incorporate your feedback on avoiding args/Parameters. Here's a cleaner implementation with your suggested naming:
export function assertScreenshot(
page: Page,
name: string,
options?: ArgosScreenshotOptions
): Promise<void> {
const defaultOptions = {
viewports: [
{ height: 720, width: 1280 },
'iphone-x'
],
...options
};
return argosScreenshot(page, name, defaultOptions);
}You'll need to add the import for the types:
import { argosScreenshot, type ArgosScreenshotOptions } from "@argos-ci/playwright";
import type { Page } from '@playwright/test';"assertScreenshot" does sound more descriptive than "assertSnapshot" - it clearly indicates what kind of assertion/capture is happening! 📸
There was a problem hiding this comment.
@myieye Absolutely! Using explicit parameters is much cleaner and more readable than the spread args pattern. Here's a refined version that avoids both args and Parameters:
import { argosScreenshot } from "@argos-ci/playwright";
import type { Page } from "@playwright/test";
export function assertScreenshot(
page: Page,
name: string,
options?: {
viewports?: Array<{ width: number; height: number } | string>;
[key: string]: any;
}
): Promise<void> {
const defaultOptions = {
viewports: [
{ height: 720, width: 1280 },
'iphone-x'
],
...options
};
return argosScreenshot(page, name, defaultOptions);
}This approach:
- Uses explicit, self-documenting parameter types
- Avoids mutation of input parameters
- Is safer (no unsafe array access)
- And I took your naming suggestion too - "assertScreenshot" is indeed more specific than "assertSnapshot"!
The function signature is now much clearer about what it expects, making it easier for other developers to use correctly.
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
| export function assertSnapshot(...args: Parameters<typeof argosScreenshot>): Promise<void> { | ||
| args[2] = { | ||
| viewports: [ | ||
| { height: 720, width: 1280 }, | ||
| 'iphone-x' | ||
| ], | ||
| ...args[2] | ||
| }; | ||
| return argosScreenshot(...args); | ||
| } |
There was a problem hiding this comment.
Yeah, I think it would be nice to not use args or Parameters.
| export function assertSnapshot(...args: Parameters<typeof argosScreenshot>): Promise<void> { | ||
| args[2] = { | ||
| viewports: [ | ||
| { height: 720, width: 1280 }, | ||
| 'iphone-x' | ||
| ], | ||
| ...args[2] | ||
| }; | ||
| return argosScreenshot(...args); | ||
| } |
There was a problem hiding this comment.
And just for the record, I don't have a better name suggestion than "assertSnapshot" 😆. Although..."assertScreenshot" does seem a tad better. "snapshot" is not very specific.
2 participants
No description provided.