-
Notifications
You must be signed in to change notification settings - Fork 164
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
base: main
Are you sure you want to change the base?
Conversation
packages/components/Button/__tests__/win32/ButtonLegacy.test.tsx
Outdated
Show resolved
Hide resolved
"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?) |
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? |
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 |
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
Hmm okay, maybe the tests shouldn't be part of the package if we're trying to reduce binary size? |
@lyzhan7 @Saadnajmi I was having trouble getting all of the following to work:
The current structure that I have the tests results in the following:
Thoughts? |
@@ -0,0 +1,2 @@ | |||
# Don't include test files in package | |||
__tests__/ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Platforms Impacted
Description of changes
This change demos what configuring and implementing multi-plat jest tests could look like.
Main things:
__tests__
file so that they're in one placesrc
because this is a lot of files and they don't need to be in the published packageVerification
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):