Skip to content
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

Change apple theme jest config platform from iOS -> macOS #2502

Merged
merged 5 commits into from
Jan 10, 2023

Conversation

lyzhan7
Copy link
Contributor

@lyzhan7 lyzhan7 commented Jan 9, 2023

Platforms Impacted

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

Description of changes

PR is related to #2481, making this a separate PR in order to keep the original as small as possible.

Why this change is required for PR 2481:

  • Currently the apple-theme tests are running on iOS, so some of the code that's getting run is iOS-specific code, but the tests are testing macOS-specific methods and are using macOS tokens
  • One of the reasons this has worked is that the mapPipelineToTheme method has been the same for both iOS/macOS
  • After 2481 we've introduced an iOS specific mapPipelineToTheme.ios.ts file, which is written for the new iOS tokens. The apple-theme tests fail after this change because now we're trying to create an iOS alias token object from macOS tokens
  • Given that the tests seem to have been written with macOS in mind, update the tests to run on the macOS.
  • Ran into an issue with updating the macOS tests, filed here Fix apple-theme jest test on macOS #2501. Justification for commenting out this test - this test hasn't been testing either macOS or iOS correctly - we've been using a mix of code from the two platforms. This test didn't run into the same issue before because it was actually running the iOS version of that code (createAppleTheme.ios.ts, instead of createAppleTheme.macos.ts) found a fix for this issue
  • Another option I considered was to change the tests to work for iOS- I would remove all the macOS specific tests (anything related to high contrast mode/dynamic). Decided it would be better to keep the testing coverage the same as before, and later find a way to add iOS-specific tests in addition to the existing macOS tests

Note: planning to write iOS tests in the future

Verification

No visual changes

Pull request checklist

This PR has considered (when applicable):

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

@lyzhan7 lyzhan7 requested a review from a team as a code owner January 9, 2023 19:45
@lyzhan7 lyzhan7 mentioned this pull request Jan 10, 2023
10 tasks
@lyzhan7 lyzhan7 merged commit 1653ac2 into microsoft:main Jan 10, 2023
@lyzhan7 lyzhan7 deleted the make-apple-theme-tests-run-on-macos branch January 10, 2023 18:18
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