Skip to content

refact: Refactor snapshot tests to conditionally skip based on options#7674

Merged
deleonio merged 1 commit intorelease/2from
7251-v2
May 11, 2025
Merged

refact: Refactor snapshot tests to conditionally skip based on options#7674
deleonio merged 1 commit intorelease/2from
7251-v2

Conversation

@deleonio
Copy link
Copy Markdown
Contributor

@deleonio deleonio commented May 9, 2025

This pull request refines the visual testing logic in theme-snapshots.spec.js to improve flexibility and efficiency by introducing conditional checks for skipping specific tests and restructuring snapshot options. The most important changes include adding conditions to skip unnecessary tests, refining snapshot options, and ensuring zoom-specific options are applied correctly.

Improvements to test skipping logic:

  • Added a conditional check to skip tests entirely if both options.snapshot.skip and options.snapshot.zoom.skip are true. This avoids running unnecessary tests. ([packages/tools/visual-tests/tests/theme-snapshots.spec.jsR24-R26](https://github.com/public-ui/kolibri/pull/7674/files#diff-2d3c27d5e93c119802fb7d5d17d32ef870f9f86071c72dda4bcd60e19b85fa62R24-R26))

Enhancements to snapshot options:

  • Refactored the snapshot options by introducing a SNAPSHOT_OPTIONS constant that merges DEFAULT_SNAPSHOT_OPTIONS with options.snapshot.options. This improves clarity and modularity. ([packages/tools/visual-tests/tests/theme-snapshots.spec.jsL48-R62](https://github.com/public-ui/kolibri/pull/7674/files#diff-2d3c27d5e93c119802fb7d5d17d32ef870f9f86071c72dda4bcd60e19b85fa62L48-R62))
  • Ensured zoom-specific snapshot options are applied by extending SNAPSHOT_OPTIONS with options.snapshot.zoom.options when generating zoomed snapshots. ([packages/tools/visual-tests/tests/theme-snapshots.spec.jsL60-R74](https://github.com/public-ui/kolibri/pull/7674/files#diff-2d3c27d5e93c119802fb7d5d17d32ef870f9f86071c72dda4bcd60e19b85fa62L60-R74))

The A11y and PO reviews will only take place after all other DoD steps have been completed by the Developer:

  • Meaningful pull request title for the release notes
  • Pull request is linked to an issue and all changes relate to the issue
  • Tests to protect this code implemented (if applicable)
  • Manual test performed successfully (if applicable)
  • Documentation or migration has been updated (if applicable)

@deleonio deleonio added the v2 label May 9, 2025
@deleonio deleonio requested a review from Copilot May 9, 2025 03:01
@deleonio deleonio linked an issue May 9, 2025 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors snapshot tests to conditionally skip execution based on test options, improving efficiency and clarity.

  • Introduces a conditional early return when both snapshot and zoom tests are marked to skip
  • Refactors snapshot options by merging DEFAULT_SNAPSHOT_OPTIONS with provided options using a new SNAPSHOT_OPTIONS constant
  • Applies zoom-specific snapshot options when running zoom tests

Comment on lines +57 to +62
if (options.snapshot?.skip === false) {
await expect(page).toHaveScreenshot(`${snapshotName}.png`, SNAPSHOT_OPTIONS);
}

// Skip unnecessary zoom tests
if (options.snapshot?.zoom?.skip === false) {
Copy link

Copilot AI May 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider revising the condition 'options.snapshot?.skip === false' to ensure that tests run by default when skip is not explicitly set to true. For instance, checking for '!== true' may help avoid inadvertently skipping tests when the option is undefined.

Suggested change
if (options.snapshot?.skip === false) {
await expect(page).toHaveScreenshot(`${snapshotName}.png`, SNAPSHOT_OPTIONS);
}
// Skip unnecessary zoom tests
if (options.snapshot?.zoom?.skip === false) {
if (options.snapshot?.skip !== true) {
await expect(page).toHaveScreenshot(`${snapshotName}.png`, SNAPSHOT_OPTIONS);
}
// Skip unnecessary zoom tests
if (options.snapshot?.zoom?.skip !== true) {

Copilot uses AI. Check for mistakes.
}

// Skip unnecessary zoom tests
if (options.snapshot?.zoom?.skip === false) {
Copy link

Copilot AI May 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Similarly, evaluate whether using 'options.snapshot?.zoom?.skip === false' is the best approach or if a check for '!== true' may be more robust, ensuring zoom tests execute by default unless explicitly skipped.

Suggested change
if (options.snapshot?.zoom?.skip === false) {
if (options.snapshot?.zoom?.skip !== true) {

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 9, 2025

Netlify Draft Deployment
URL:
Logs:

@deleonio deleonio force-pushed the 7251-v2 branch 2 times, most recently from fc8f33e to d403785 Compare May 9, 2025 03:29
@deleonio deleonio marked this pull request as ready for review May 11, 2025 04:10
@deleonio deleonio merged commit 76cad9c into release/2 May 11, 2025
9 of 10 checks passed
@deleonio deleonio deleted the 7251-v2 branch May 11, 2025 04:11
@publicuibot publicuibot bot locked and limited conversation to collaborators May 11, 2025
@deleonio deleonio removed the v2 label Jun 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Visuelle Tests: Snapshots reduzieren

2 participants