Skip to content
This repository has been archived by the owner on Jan 10, 2025. It is now read-only.

Migrate testing setup to use babel-jest #937

Merged
merged 6 commits into from
Sep 5, 2019
Merged

Conversation

ismail-syed
Copy link
Contributor

@ismail-syed ismail-syed commented Aug 30, 2019

Description

Related to #857

This PR migrates quilts testing setup to use babel-jest over ts-jest. This is because ts-jest has a hard time understanding TS project references. This also makes reviewing the subsequent PR to address #857 a bit easier.

In summary:

  • Upgrading to jest 24
  • Adding babel & babel-jest
  • Get CI 🍏

@ismail-syed ismail-syed changed the title Migrate to babel jest Migrate testing setup to use babel-jest Aug 30, 2019
@ismail-syed
Copy link
Contributor Author

ismail-syed commented Aug 30, 2019

CI is failing because FieldName is undefined. @shopify/address-consts isn't being picked up in this export and results in undefined. Changing the export to export * from '../../address-consts'; fixes it.

When poking around locally, the package is infact there and symlinked to the folder. It's also referenced as expected in the moduleNameMapper jest config.

Fixed: See comment below

@ismail-syed ismail-syed self-assigned this Aug 30, 2019
@ismail-syed ismail-syed force-pushed the migrate-to-babel-jest branch 2 times, most recently from b9d8e4d to 55c29d8 Compare September 4, 2019 21:46
@ismail-syed ismail-syed force-pushed the migrate-to-babel-jest branch from fc389f4 to 53b9920 Compare September 5, 2019 13:25
jest.config.js Outdated
@@ -16,15 +16,16 @@ const moduleNameMapper = getPackageNames().reduce(
'<rootDir>/packages/react-html/src/server/index.ts',
'@shopify/react-network/server':
'<rootDir>/packages/react-network/src/server.ts',
'@shopify/address-consts': '<rootDir>/packages/address-consts/src/index.ts',
Copy link
Contributor Author

@ismail-syed ismail-syed Sep 5, 2019

Choose a reason for hiding this comment

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

@shopify/address-conts was not getting module mapped properly.
require.resolve('@shopify/address-consts') was resolving to quilt/packages/address/src/index.ts, instead of quilt/packages/address-consts/src/index.ts. Moving this up in the list manually got the module resolving correctly.

Copy link
Member

Choose a reason for hiding this comment

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

I think we could also fix this by adding a $ to the @shopify/${name} bit, presumably that part is a regex and address matches address-consts

@ismail-syed ismail-syed marked this pull request as ready for review September 5, 2019 13:34
Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

This looks awesome 🥇

jest.config.js Outdated
@@ -16,15 +16,16 @@ const moduleNameMapper = getPackageNames().reduce(
'<rootDir>/packages/react-html/src/server/index.ts',
'@shopify/react-network/server':
'<rootDir>/packages/react-network/src/server.ts',
'@shopify/address-consts': '<rootDir>/packages/address-consts/src/index.ts',
Copy link
Member

Choose a reason for hiding this comment

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

I think we could also fix this by adding a $ to the @shopify/${name} bit, presumably that part is a regex and address matches address-consts

@ismail-syed ismail-syed force-pushed the migrate-to-babel-jest branch from 774bd2f to 0b807cf Compare September 5, 2019 17:24
@ismail-syed ismail-syed merged commit cfb41d1 into master Sep 5, 2019
@cartogram cartogram temporarily deployed to production September 5, 2019 23:16 Inactive
@GoodForOneFare GoodForOneFare temporarily deployed to gem September 18, 2019 14:23 Inactive
@BPScott BPScott deleted the migrate-to-babel-jest branch May 22, 2021 00:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants