-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(testing): update jest-preset-angular to v8.0.0 #2250
feat(testing): update jest-preset-angular to v8.0.0 #2250
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks 💙, @mehrad-rafigh. I hope this PR been merged as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
Can you please keep the commit by @JoshMentzer so his work is attributed to him?
const jestConfigsToUpdate = []; | ||
|
||
Object.keys(workspaceConfig.projects).forEach(name => { | ||
const project = workspaceConfig.projects[name]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should go through all of the architect targets.. for targets which use @nrwl/jest:jest
as the builder... and use the jestConfig
option verbatim if it exists so that we can catch all jest.config.js
which are referenced in workspace.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get your point. Is something not correct here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, let me clarify, this only migrates targets which have a test
target that uses @nrwl/jest:jest
.
A user could have the following:
projects: {
app1: {
architect: {
e2e: {
builder: '@nrwl/jest:jest',
options: { ... }
}
}
}
Such projects should be migrated as well.
Hi @FrozenPandaz. Would you please review the changes I made? We really need this in nx |
@FrozenPandaz pinging you again, hoping you will review this :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think having him as a co-author of your commit will persist after someone merges this PR because it assigns the merger as the co-author?
Can you have his commit there separately and we won't squash when we merge? This likely involves remaking your branch.. cherry-picking his PR and then adding your changes on top as a separate commit...
const jestConfigsToUpdate = []; | ||
|
||
Object.keys(workspaceConfig.projects).forEach(name => { | ||
const project = workspaceConfig.projects[name]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, let me clarify, this only migrates targets which have a test
target that uses @nrwl/jest:jest
.
A user could have the following:
projects: {
app1: {
architect: {
e2e: {
builder: '@nrwl/jest:jest',
options: { ... }
}
}
}
Such projects should be migrated as well.
@FrozenPandaz I will take a look tonight. If I can cherry pick the commit without too much hustle, I will do it. |
Sorry to mess with the discussion in this PR, but it is no big deal to incorporate the original commit.
Even if you do not want to go on with this PR anymore, this way someone else can continue with your work, and you will still author your work in your second commit. Cheers and happy coding! |
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
First of all, happy new year to all of you,
This PR supersedes #1986 due to inactivity and closes #1979 and #2165
I added the new snapshot serializer that was mentioned by @wtho and updated everything accordingly.
Hoping we can merge this asap :)