refactor(jest): add internal support for jest 27#3171
Merged
rwaskiewicz merged 3 commits intomainfrom Dec 8, 2021
Merged
Conversation
d83250a to
e631cc5
Compare
this commit adds support for stencil to run jest 27 for its own unit tests. although it does not enable jest 27 for consumers of stencil, it is the first step in the process that future work shall build upon. update `jest`, `jest-cli` and `jest-environment-node` all to v27. jest v27 switched many defaults that would otherwise break stencil tests. to support jest v27: - explicitly use the 'jest-jasmine2' test runner in our own internal configuration. moving to 'jest-circus' will occur in STENCIL-307 and is not on the critical path for supporting Jest 27 - create a `getVmContext` function in jest environment setup, a new field that is required in v27 - remove unneeded `async` keyword from telemetry tests, which were causing jest to fail in addition to the changes above, several changes were required to be made to our `jest-preprocessor`, which is used both by consumers of stencil in addition to internal stencil tests: Between Jest v23 (the latest when this file was originally authored) and v27, the function signatures of `process` and `getCacheKey` have changed, most ostenibly between v26 and v27. for the purposes of this PR, we attempt to maintain backwards compatibility to the best of our ability, which leads to extra type checking. it is expected that this be removed in a future major version of Stencil. The context of `this` has changed between v26 and v27 when code coverage is generated for files that do not have tests associated with them. in the original implementation, running jest with the '--coverage' flag would cause a message to be printed to STDERR for _every file_ that didn't have test coverage. To circumvent this, cached instances of `tsconfig.json#options` and its stringified version have been hoisted to no longer require being referenced through `this`. An integer used to bust the Jest cache (previously just the number '6') has been refactored out to a constant, `CACHE_BUSTER` with a JSDoc to explain its usage and when it should be updated STENCIL-61: getCacheKey rework to support Jest 27 STENCIL-63: Jest 27 support for @stencil/core
e631cc5 to
d715124
Compare
disable the tests that are responsible for invoking jest as a part of the testing suite. the test assumes that jest can be run programmatically, specifically that the program has programmatic access to jest cli flags. with jest 27, this is no longer true. to limit the scope of this work (which is planned in STENCIL-71), temporarily disable the test. this is only necessary because stencil is internally running v27, but end users are still using v26.
f00dfa6 to
fb1efa2
Compare
willmartian
approved these changes
Dec 8, 2021
Contributor
willmartian
left a comment
There was a problem hiding this comment.
Looks good, nice work. Followed the test directions in the PR description and everything seems to be working as intended.
Additionally, I tested npm run test.screenshot in src/core locally and received no test runner errors. The screenshot tests themselves did fail, however, but this is consistent with main on Ionic Framework and not related to this PR as far as I can tell.
Member
Author
|
Thanks @willmartian - part of that on my end may be related to running a newer version of Puppeteer (due to some compatibility issues with M1 machines), much appreciated for double checking 🙏 |
This was referenced Dec 16, 2021
Closed
14 tasks
rwaskiewicz
added a commit
that referenced
this pull request
Mar 7, 2022
When we upgraded Stencil to internally use Jest 27 (#3171), we began to see warnings in the console that look something like: > ERROR: 'DEPRECATION: An asynchronous before/it/after function took a done callback but also returned a promise. This is not supported and will stop working in the future. Either remove the done callback (recommended) or change the function to not return a promise. (in spec: SPEC NAME)' This commit removes either the usage of the `done` parameter or `async` keyword from tests, depending on what was required/based on the structure of the test itself
14 tasks
pull bot
pushed a commit
to digitty-forks/stencil
that referenced
this pull request
Aug 20, 2022
this commit removes a handful of strict null check violations that can be found in stencil's jest preprocessor. most changes are concerned with making types agree - e.g. if a variable is declared to be of type `T`, but is assigned by a function that returns `T | U`, the type of the variable is expanded for agreement. the two changes that are worth calling out in particular is introducing a `throw` statement in the event that the `transformOptions` parameter is falsy both when we process a file and attempt to retrieve a cache key. from jest 23 thru 27, the function signatures of these two functions changed a few times (documented in the code today). however, a typing issue that was introduced in e0640de (stenciljs#3171) was `transformOptions` could be considered `undefined` by typescript. we know that for jest [23,26], this value will never be falsy (as it is a part of the call signature). for jest 27, we re-assign a value to this variable for backwards compatibility reasons. there is a high level of confidence that this `throw` is safe to introduce for the following reasons: 1. if `transformOptions` is undefined/null, stencil would already be erroring out when running the test task 2. the function signatures changes of these two functions have been handled in e0640de 3. in jest [23,26], `transformOptions` _always_ has a `rootDir` field of type `string` (which is the field we're accessing here causing errors)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm test) were run locally and passednpm run test.karma.prod) were run locally and passednpm run prettier) was run locally and passedPull request type
Please check the type of change your PR introduces:
What is the current behavior?
GitHub Issue Number: N/A
What is the new behavior?
this commit adds support for stencil to run jest 27 for its own unit
tests. although it does not enable jest 27 for consumers of stencil, it
is the first step in the process that future work shall build upon.
update
jest,jest-cliandjest-environment-nodeall to v27. jestv27 switched many defaults that would otherwise break stencil tests. to
support jest v27:
configuration. moving to 'jest-circus' will occur in STENCIL-307 and
is not on the critical path for supporting Jest 27
getVmContextfunction in jest environment setup, a newfield that is required in v27
asynckeyword from telemetry tests, which werecausing jest to fail
in addition to the changes above, several changes were required to be
made to our
jest-preprocessor, which is used both by consumers ofstencil in addition to internal stencil tests:
Between Jest v23 (the latest when this file was originally authored) and
v27, the function signatures of
processandgetCacheKeyhavechanged, most ostensibly between v26 and v27. for the purposes of this
PR, we attempt to maintain backwards compatibility to the best of our
ability, which leads to extra type checking. it is expected that this be
removed in a future major version of Stencil.
The context of
thishas changed between v26 and v27 when code coverageis generated for files that do not have tests associated with them. in
the original implementation, running jest with the '--coverage' flag
would cause a message to be printed to STDERR for every file that
didn't have test coverage. To circumvent this, cached instances of
tsconfig.json#optionsand its stringified version have been hoisted tono longer require being referenced through
this.An integer used to bust the Jest cache (previously just the number '6')
has been refactored out to a constant,
CACHE_BUSTERwith a JSDoc toexplain its usage and when it should be updated
Does this introduce a breaking change?
Testing
Almost all Stencil tests pass in this branch. One test was explicitly disabled. The reason for which being is that between Jest v26 and v27, CLI args for the library are no longer publicly exposed. This is a known issue with how Jest + Stencil integrate with one another, and the work was previously planned to be done in STENCIL-71. To avoid adding additional work/scope to this PR, the test is temporarily disabled. *
In addition, this library was tested in the Ionic Framework by building this branch (
npm ci && npm run build && npm pack) and performing the following in order:Ionic Core Tests
ionic-framework/corenpm run testwas run to invoke spec and e2e tests, all of which passednpm run test.screenshotwas not run to invoke screenshot tests as I get failures using an older version of Puppeteer on Framework on an M1 macnpm run test.treeshakewas run