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

[Proposal] Jest multi plat testing #2506

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

rurikoaraki
Copy link
Collaborator

@rurikoaraki rurikoaraki commented Jan 10, 2023

Platforms Impacted

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

Description of changes

This change demos what configuring and implementing multi-plat jest tests could look like.

Main things:

  • I'm using Button as the component of choice
  • I moved all the tests under a __tests__ file so that they're in one place
  • I moved all the tests out of src because this is a lot of files and they don't need to be in the published package
  • I added a jest.config file for each platform
  • I added test files for each platform. I think this is unfortunately necessary to get the snapshot comparison to work per-platform, because the path to the snapshot file for a test has to be able to be derived from the path to the test file, and vice versa. See this for more info on the API.
  • Snapshots were added for each platform
  • yarn test now calls yarn test-iOS and yarn-win32, which then find the appropriate config file
  • jest task has been modified to account for -c arg

Verification

Verified yarn test runs both sets of tests locally. win32 doesn't pass at this moment.

Pull request checklist

This PR has considered (when applicable):

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

@lyzhan7
Copy link
Contributor

lyzhan7 commented Jan 11, 2023

"I moved all the tests out of src because [...] they don't need to be in the published package" - I didn't know that everything outside of src gets removed from the published package. Is that behavior something we specified for FURN or is that from something else (yarn maybe?)

@lyzhan7
Copy link
Contributor

lyzhan7 commented Jan 11, 2023

I was wondering if you've looked into the rnx-kit/jest-preset package to see if anything there would be useful? It looks like they add the ability to specify multiple jest.config.js files, like jest.config.ios.js - not exactly sure what this is meant for though, is it so that you can have tests on different platforms or is it just so that locally when you run a test it automatically chooses the platform you're on

@rurikoaraki
Copy link
Collaborator Author

rurikoaraki commented Jan 11, 2023

I was wondering if you've looked into the rnx-kit/jest-preset package to see if anything there would be useful? It looks like they add the ability to specify multiple jest.config.js files, like jest.config.ios.js - not exactly sure what this is meant for though, is it so that you can have tests on different platforms or is it just so that locally when you run a test it automatically chooses the platform you're on

https://microsoft.github.io/rnx-kit/docs/tools/jest-preset#multiple-jestconfigjs

This looks basically like what this PR is setting up. Since we have snapshot files I don't think it's enough to only fork the jest files per endpoint. ETA: Were you suggesting something else?

@rurikoaraki
Copy link
Collaborator Author

"I moved all the tests out of src because [...] they don't need to be in the published package" - I didn't know that everything outside of src gets removed from the published package. Is that behavior something we specified for FURN or is that from something else (yarn maybe?)

Looking at it again I was wrong, the files will get published, but they won't be included in the lib/lib-commonjs files (i.e. transpiled from TS to JS). This is specified https://github.com/microsoft/fluentui-react-native/blob/main/scripts/src/tasks/ts.js I think

@lyzhan7
Copy link
Contributor

lyzhan7 commented Jan 11, 2023

I was wondering if you've looked into the rnx-kit/jest-preset package to see if anything there would be useful? It looks like they add the ability to specify multiple jest.config.js files, like jest.config.ios.js - not exactly sure what this is meant for though, is it so that you can have tests on different platforms or is it just so that locally when you run a test it automatically chooses the platform you're on

https://microsoft.github.io/rnx-kit/docs/tools/jest-preset#multiple-jestconfigjs

This looks basically like what this PR is setting up. Since we have snapshot files I don't think it's enough to only fork the jest files per endpoint. ETA: Were you suggesting something else?

Ah no I guess I was thinking maybe the way the jest files got forked per endpoint could help with getting snapshot comparison to work per platform but maybe not

"I moved all the tests out of src because [...] they don't need to be in the published package" - I didn't know that everything outside of src gets removed from the published package. Is that behavior something we specified for FURN or is that from something else (yarn maybe?)

Looking at it again I was wrong, the files will get published, but they won't be included in the lib/lib-commonjs files (i.e. transpiled from TS to JS). This is specified https://github.com/microsoft/fluentui-react-native/blob/main/scripts/src/tasks/ts.js I think

Hmm okay, maybe the tests shouldn't be part of the package if we're trying to reduce binary size?

@rurikoaraki
Copy link
Collaborator Author

rurikoaraki commented Feb 18, 2023

@lyzhan7 @Saadnajmi I was having trouble getting all of the following to work:

  1. Only one copy of the test pages across all the platforms
  2. Snapshot files being found correctly per platform
  3. Snapshots from other platforms not being reported as obsolete. Jest has functionality that basically keeps track of whether snapshot files have been checked while running tests, and if it finds any snapshot files that aren't used, it'll mark them as obsolete and exit with an error code.
  4. Snapshots being separated per test page

The current structure that I have the tests results in the following:

  1. Only one copy of the tests, but they are all wrapped in a function so that they can be exported and run in a different file
  2. Each platform has its own main test page and jest config. This allows the snapshots for each platform to not affect other platforms, because jest will only try to find snapshot files that are in the subtree of its source directory
  3. Side effect of the current setup is that all the snapshots are in one file. If we want them to be in separate files, then we'll need to have a File.test.tsx file for each test file under each platform folder. The content could still be shared crossplat but it feels to me that it would add a lot of clutter.

Thoughts?

@@ -0,0 +1,2 @@
# Don't include test files in package
__tests__/
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we need this in every package, can we also update the component-generator later, if not in this PR? I know that isn't heavily used but I think some people may still try to use that as their "first new package" template

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, that makes sense. I can also add it top level instead.

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.

3 participants