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

fix(jest-config): normalize reporters option defined in presets #12769

Merged
merged 5 commits into from Apr 29, 2022

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Apr 29, 2022

Fixes #12767

Summary

As it was described in the issue, reporters option defined in a preset is not normalized properly. Looking at jest-config, one can see that preset options are merged after reporters option is normalized. To solve the issue normalization logic has be done later.

Test plan

Unit test is added.

@mrazauskas mrazauskas marked this pull request as draft April 29, 2022 07:15
{} as Config.Argv,
);

expect(options.reporters).toEqual([['summary', {}]]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm.. reporters from preset is discarded if config includes reporters key. Just thinking if this expected? Or it would be better to merge? Merging probably would make it impossible to disable reporters from a preset.

Edit: I just added a test, there is no change in behaviour. The test is passing on main as well.

Copy link
Member

Choose a reason for hiding this comment

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

not sure if ideal, but not something we should change here anyways

@mrazauskas mrazauskas marked this pull request as ready for review April 29, 2022 07:24
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

good stuff 👍

We really should have runtime verification of the types out of normalize... (@jest/schema is supposed to enable that)

@@ -383,7 +383,7 @@ module.exports = {
'no-dupe-args': 'error',
'no-dupe-class-members': 'error',
'no-dupe-keys': 'error',
'no-duplicate-case': 'warn',
'no-duplicate-case': 'error',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps? I just missed that warning, better if it errors?

Copy link
Member

Choose a reason for hiding this comment

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

agreed, that's always an error I think

@SimenB SimenB merged commit 3309659 into jestjs:main Apr 29, 2022
@mrazauskas mrazauskas deleted the fix-normalizeReporters branch April 29, 2022 09:52
F3n67u pushed a commit to F3n67u/jest that referenced this pull request May 2, 2022
@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 30, 2022
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.

[Bug]: An error occurred while adding the reporter at path "d"
3 participants