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

Upgrade to Jest 23.5.0 #22791

Merged
merged 10 commits into from
Sep 8, 2018
Merged

Upgrade to Jest 23.5.0 #22791

merged 10 commits into from
Sep 8, 2018

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Sep 7, 2018

I'd really like to upgrade to Typescript 3 for its unknown type, but we need to upgrade to jest@23 to support a recent version of ts-jest@23.

The jest changelog breaks down the breaking changes in 23.x, but I found it to be slightly incomplete so I've broken down the changes that actually caused breaks for us here, and addressed each in individual commits to make review a little easier:

  • the testURL config default was changed from about:blank to http://localhost
    • this cause some XHR requests powered by JSdom to start failing. It seems these requests just do nothing in master but start to fail when JSdom is initialized with an actual URL... I think we would ideally stop sending meaningless XHR requests in the tests, but it was a lot easier to just set the config to about:blank for now, and we can worry about cleanup later if necessary
  • expect(...).toThrow() only passes if an actual error was thrown.
    • In two places in the index pattern code we were throwing strings, which broke the assertions. Fortunately/Unfortunately the errors are not being consumed by anything, so I was able to wrap them in new Error() without causing any issues.
  • snapshots of mock functions now include a results array, detailing the return values of the function
  • React fragments are now serialized as <React.Fragment> instead of <UNDEFINED>
  • undefined props in React components are now stripped from snapshots
  • minor changes to the ordering of mocks, imports resolution, and before hooks caused the uiSettings API tests to start breaking, but I'm replacing them with totally new tests in Migrate base path APIs and UiSettings client to new platform #22694 so I just deleted them here
  • mocks created with jest.spyOn() that are restored now have their mock.calls reset, so some of the kbn-pm tests stated failing. This was fixed by restoring them with jest.restoreAllMocks() rather than trying to do it before the assertions

@spalger spalger added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.5.0 labels Sep 7, 2018
@spalger spalger requested a review from azasypkin September 7, 2018 00:53
@spalger spalger changed the title Upgrade/jest Upgrade to Jest 23.5.0 Sep 7, 2018
@spalger spalger added the review label Sep 7, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@azasypkin
Copy link
Member

ack: started to look into this one, if don't finish today will continue on Monday (I'm on "Support Duty" on Monday though, but we'll see).

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Okay, review was easier than I thought initially. I really wanted Jest's new mock.returnResults, so thanks a lot for upgrading it!

There is a chance that this PR will conflict with other PRs that are going to be merged today and won't be rebased on the latest master, so we probably need an announcement :)

@@ -42,7 +42,7 @@ export async function getIndices(es, rawPattern, limit) {

// We need to always provide a limit and not rely on the default
if (!limit) {
throw '`getIndices()` was called without the required `limit` parameter.';
throw new Error('`getIndices()` was called without the required `limit` parameter.');
Copy link
Member

Choose a reason for hiding this comment

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

👍

@spalger spalger merged commit 7e94ecc into elastic:master Sep 8, 2018
spalger pushed a commit to spalger/kibana that referenced this pull request Sep 8, 2018
I'd really like to upgrade to Typescript 3 for its `unknown` type, but we need to upgrade to `jest@23` to support a recent version of `ts-jest@23`.

The [jest changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md) breaks down the breaking changes in 23.x, but I found it to be slightly incomplete so I've broken down the changes that actually caused breaks for us here, and addressed each in individual commits to make review a little easier:

- the `testURL` config default was changed from `about:blank` to `http://localhost`
    - this cause some XHR requests powered by JSdom to start failing. It seems these requests just do nothing in master but start to fail when JSdom is initialized with an actual URL... I think we would ideally stop sending meaningless XHR requests in the tests, but it was a lot easier to just set the config to `about:blank` for now, and we can worry about cleanup later if necessary
- `expect(...).toThrow()` only passes if an actual error was thrown.
     - In two places in the index pattern code we were throwing strings, which broke the assertions. Fortunately/Unfortunately the errors are not being consumed by anything, so I was able to wrap them in `new Error()` without causing any issues.
- snapshots of mock functions now include a `results` array, detailing the return values of the function
- React fragments are now serialized as `<React.Fragment>` instead of `<UNDEFINED>`
- undefined props in React components are now stripped from snapshots
- minor changes to the ordering of mocks, imports resolution, and before hooks caused the uiSettings API tests to start breaking, but I'm replacing them with totally new tests in elastic#22694 so I just deleted them here
- mocks created with `jest.spyOn()` that are restored now have their `mock.calls` reset, so some of the kbn-pm tests stated failing. This was fixed by restoring them with `jest.restoreAllMocks()` rather than trying to do it before the assertions
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

spalger pushed a commit that referenced this pull request Sep 8, 2018
I'd really like to upgrade to Typescript 3 for its `unknown` type, but we need to upgrade to `jest@23` to support a recent version of `ts-jest@23`.

The [jest changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md) breaks down the breaking changes in 23.x, but I found it to be slightly incomplete so I've broken down the changes that actually caused breaks for us here, and addressed each in individual commits to make review a little easier:

- the `testURL` config default was changed from `about:blank` to `http://localhost`
    - this cause some XHR requests powered by JSdom to start failing. It seems these requests just do nothing in master but start to fail when JSdom is initialized with an actual URL... I think we would ideally stop sending meaningless XHR requests in the tests, but it was a lot easier to just set the config to `about:blank` for now, and we can worry about cleanup later if necessary
- `expect(...).toThrow()` only passes if an actual error was thrown.
     - In two places in the index pattern code we were throwing strings, which broke the assertions. Fortunately/Unfortunately the errors are not being consumed by anything, so I was able to wrap them in `new Error()` without causing any issues.
- snapshots of mock functions now include a `results` array, detailing the return values of the function
- React fragments are now serialized as `<React.Fragment>` instead of `<UNDEFINED>`
- undefined props in React components are now stripped from snapshots
- minor changes to the ordering of mocks, imports resolution, and before hooks caused the uiSettings API tests to start breaking, but I'm replacing them with totally new tests in #22694 so I just deleted them here
- mocks created with `jest.spyOn()` that are restored now have their `mock.calls` reset, so some of the kbn-pm tests stated failing. This was fixed by restoring them with `jest.restoreAllMocks()` rather than trying to do it before the assertions
@spalger
Copy link
Contributor Author

spalger commented Sep 8, 2018

6.x/6.5: 9a7fa9a

@spalger spalger deleted the upgrade/jest branch September 8, 2018 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants