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

Make it possible to merge transform option with preset #5505

Merged
merged 4 commits into from
Feb 17, 2018
Merged

Make it possible to merge transform option with preset #5505

merged 4 commits into from
Feb 17, 2018

Conversation

gziolo
Copy link
Contributor

@gziolo gziolo commented Feb 9, 2018

Summary

I discovered that transform option set in Jest preset is completely overridden when the same option is provided in the Jest config that extends such preset. I run into this issue when working on the preset for WordPress core: WordPress/gutenberg#4705. It turned out that behavior differs from another option that uses object as data representations: moduleNameMapper. This PR tries to introduce consistency in the code and allow to merge values from transform option set in both config and preset.

Example

@wordpress/jest-preset-default:

{
  "transform": {
    "^.+\\.jsx?$": "babel-jest",
    "\\.pegjs$": "<rootDir>/node_modules/@wordpress/jest-preset-default/scripts/pegjs-transform.js
  } 
}

jest.config.js:

{
  "preset": "@wordpress/jest-preset-default",
  "transform": {
    "\\.pegjs$": "<rootDir>/test/unit/pegjs-transform.js"
  }
}

Expected output:

{
  "transform": {
    "^.+\\.jsx?$": "babel-jest",
    "\\.pegjs$": "<rootDir>/test/unit/pegjs-transform.js"
  }
}

Test plan

This change is limited to `jest-config. This is how it was tested:

yarn test packages/jest-config

@gziolo gziolo changed the title Make it possible to merge transform options and preset Make it possible to merge transform option and preset Feb 9, 2018
@thymikee
Copy link
Collaborator

thymikee commented Feb 9, 2018

Can you add a test for that? Merging transforms may cause unexpected issues though, like we had with moduleNameMapper (hence this weird merging logic as you can see in a block above your code)

@gziolo
Copy link
Contributor Author

gziolo commented Feb 9, 2018

Sure thing, I wanted to have PR to reference in the PR I'm working on. I have to override this option to make it work which is pretty unexpected when using preset. Well, it might be not that confusing if some options wouldn't work this way :)

I must admit that moduleNameMapper is handled in a very unexpected way. Why is there needed 1st options.moduleNameMapper occurrence? It's always replaced by the 2nd occurrence of the same object.

I think we can also provide some helper functions to make this easier to maintain:

const mergeOptionWithPreset = (options, preset, optionName) => {
  if (options[optionName] && preset[optionName]) {
    options[optionName] = Object.assign(
      {},
      preset[optionName],
      options[optionName],
    );
  }
}  

It can be expressed also in a more functional way if you prefer.

@thymikee
Copy link
Collaborator

thymikee commented Feb 9, 2018

See #3689

@gziolo
Copy link
Contributor Author

gziolo commented Feb 9, 2018

So it's only about the order. It makes sense then. I will follow the same pattern. It still makes sense to do some refactoring, should I include it in this PR or open another one?

@thymikee
Copy link
Collaborator

thymikee commented Feb 9, 2018

This is the same logic, so feel free to refactor it as a part of this PR, it's small enough to fit in.

@@ -964,7 +995,7 @@ describe('preset without setupFiles', () => {

beforeAll(() => {
jest.mock(
'/node_modules/react-native/jest-preset.json',
'/node_modules/react-foo/jest-preset.json',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be some kind of bug here. When it mocks the same file name as in the other describe block, it doesn't reload the config but uses what was previously set. I updated this name to ensure that setupFiles is configured properly, i.e. babel-jest is enabled which triggers prepend of regenerator-runtime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could switch to jest.doMock here to avoid this, since it's already placed before the require call and we don't need to hoist it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try it. This was my guess that jest.mock calls get hoisted and this flow doesn't work as expected.

@@ -46,6 +46,21 @@ const PRESET_NAME = 'jest-preset' + JSON_EXTENSION;
const createConfigError = message =>
new ValidationError(ERROR, message, DOCUMENTATION_NOTE);

const mergeOptionWithPreset = (
options: InitialOptions,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope it is fine. This is the first time I update code that uses Flow :)

@@ -922,7 +924,10 @@ describe('preset', () => {
expect(options.setupFiles.sort()).toEqual([
'/node_modules/a',
'/node_modules/b',
'/node_modules/regenerator-runtime/runtime',
]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

babel-jest isn't explicitly set, so it doesn't append regenerator-runtime anymore.

@gziolo
Copy link
Contributor Author

gziolo commented Feb 11, 2018

@thymikee - I refactored code, added a new test and updated existing tests to better reflect the current behavior. There are some issues with cache on CircleCi, but I don't have permissions to reset them. Let me know if there is anything else to fix or add.

// Object initializer not used for properties as a workaround for
// sort-keys eslint rule while specifying properties in
// non-alphabetical order for a better test
const transform = {};
Copy link
Collaborator

@thymikee thymikee Feb 11, 2018

Choose a reason for hiding this comment

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

You can just disable the rule for this object / line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I shamelessly copied from the other test, but it makes sense what you said. I will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 👍

Copy link
Collaborator

@thymikee thymikee 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 good already 👍I wasn't able to rerun all the jobs without cache, just a couple (not sure why)

@gziolo
Copy link
Contributor Author

gziolo commented Feb 12, 2018

I tried with 757bc3b .doMock & dontMock pair, but it didn't help much. I left them to make it more obvious that they should be used only for those specific describe blocks, thus shouldn't be hoisted.

@thymikee
Copy link
Collaborator

normalize.test.js passes on Appveyor, so that should be ok.

@SimenB
Copy link
Member

SimenB commented Feb 17, 2018

@gziolo mind updating the changelog?

@gziolo
Copy link
Contributor Author

gziolo commented Feb 17, 2018

Yes, I wasn’t aware of that. I will do it by Monday 👍

@gziolo gziolo changed the title Make it possible to merge transform option and preset Make it possible to merge transform option with preset Feb 17, 2018
@gziolo
Copy link
Contributor Author

gziolo commented Feb 17, 2018

@SimenB done ✅

@SimenB
Copy link
Member

SimenB commented Feb 17, 2018

I can confirm the full test suite passes on my machine (a mac), so no issues from the skipped windows tests either

@cpojer cpojer merged commit e89de4e into jestjs:master Feb 17, 2018
{},
options[optionName],
preset[optionName],
options[optionName],
Copy link
Member

Choose a reason for hiding this comment

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

why do we have options[optionName] twice? I realise it was the same before this patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To preserve the order of keys in the config that is overriding preset. I had the same question initially 😃 Maybe an inline comment with explanation would help?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think so.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants