Skip to content

Rename Jest snapshots to be platform agnostic #1247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 14, 2021

Conversation

Saadnajmi
Copy link
Collaborator

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

Description of changes

Previously, most of our snapshot tests were of the form <component>.test.win32.tsx . As it turns out, this doesn't actually test win32 specifically, jest doesn't use the platform suffix to test per platform. When I did a console.log(Platform) inside a win32 jest test (running on a macOS machine), it printed out android. This behavior seems to be inline with this upstream issue (jestjs/jest#1370 (comment)) that confirms that jest doesn't actually run tests platform specifically.

As such, let's rename our snapshot tests to make it more clear they are actually not win32 tests, but platform agnostic tests, seemingly defaulting to either iOS or Android (not sure which one, I've seen conflicting reports). Worth noting the issue I linked details a solution to get platform specific tests, but that seemed like a larger change than what I wanted to achieve with this PR.

Verification

yarn test locally passes. CI passing should be good enough confirmation.

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

@Saadnajmi Saadnajmi changed the title Rename Jest snapshots to be platform Agnostic Rename Jest snapshots to be platform agnostic Dec 14, 2021
@Saadnajmi Saadnajmi merged commit e597717 into microsoft:master Dec 14, 2021
@Saadnajmi Saadnajmi deleted the jest branch December 14, 2021 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants