Skip to content

Conversation

@thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Mar 17, 2025

Issue number: N/A


What is the current behavior?

Certain custom commands related to Playwright are failing. Specifically:

  • npm run test.e2e.update-snapshots ${component}
  • npm run test.e2e.docker.update-snapshots ${component}

The failures are due to Playwright v1.50 changing it's accepted values for update-snapshots. Possible values are "all", "changed", "missing", and "none". By default, it's "all". All snapshots will be updated when no value is passed or a wrong value is passed.

Our scripts end up running (doesn't matter if it's local or through the docker):

  • npx playwright test --update-snapshots ${component}

It used to only update the snapshots that have changed. However, with the update, the argument thinks that the component is its value but since it's not an accepted one, it leads to the default.

What is the new behavior?

Set our default value when it comes to updating snapshots. By using changed then we would only update snapshots for those tests that have a difference. This aligns with how it used to work before the update.

Does this introduce a breaking change?

  • Yes
  • No

Other information

N/A

@vercel
Copy link

vercel bot commented Mar 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 17, 2025 6:44pm

@github-actions github-actions bot added the package: core @ionic/core package label Mar 17, 2025
@thetaPC thetaPC marked this pull request as ready for review March 17, 2025 19:57
@thetaPC thetaPC requested a review from a team as a code owner March 17, 2025 19:57
@thetaPC thetaPC requested review from brandyscarney and gnbm and removed request for gnbm March 17, 2025 19:57
Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

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

Confirmed that this correctly adds or updates screenshots when a component path is passed for:

  • A new e2e test
  • A new e2e test with updates made after creating the screenshots
  • An existing e2e test with significant changes to the component (e.g. changing background color from blue to red)

However, this does not work for:

  • An existing e2e test with minor changes to the component (e.g. adjusting a stepped color to the next closest shade):
    • I changed the input helper text color from $text-color-step-300 to $text-color-step-400

The only way to detect this type of change would be by using all, but doing so would re-generate screenshots even when no actual visual differences exist for those components. Given this, I'm okay with moving forward with this fix as is. 👍

@brandyscarney brandyscarney changed the title fix(snapshots): use default value for cmd line argument chore(snapshots): use default value for cmd line argument Mar 17, 2025
@thetaPC thetaPC changed the title chore(snapshots): use default value for cmd line argument test(snapshots): use default value for cmd line argument Mar 17, 2025
@thetaPC thetaPC changed the title test(snapshots): use default value for cmd line argument chore(snapshots): use default value for cmd line argument Mar 17, 2025
@thetaPC thetaPC added this pull request to the merge queue Mar 17, 2025
Merged via the queue into main with commit d36283e Mar 17, 2025
55 checks passed
@thetaPC thetaPC deleted the snapshots-fix branch March 17, 2025 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants