-
-
Notifications
You must be signed in to change notification settings - Fork 32.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
[test] Migrate colorManipulator from assert to expect #20792
[test] Migrate colorManipulator from assert to expect #20792
Conversation
Details of bundle changes.Comparing: 72cd216...7f97d98 Details of page changes
|
Should we imagine a codemod to migrate the whole codebase at once? Something like https://github.com/devenbansod/jest-expect-codemod? |
Yes, can be an idea but sometimes can be useful to review the actual code. |
@marcosvega91 Does a codemod prevent reviewing the actual code? What do you mean by "review"? |
@oliviertassinari if you find a problem during refactoring you can change the code and make it better |
@marcosvega91 My counter-argument would be:
Now I wonder, what's the "opportunity cost" of 2? |
I think that the second is not necessary so it can be if there is time |
Cool, do you want to look into how we can adapt https://github.com/devenbansod/jest-expect-codemod/blob/master/transformations/chaiAssert.js to cover our needs? I would hope 1-2 hours are required to write the codemod and migrate the whole codebase. But maybe they are edge cases that will be challenging. |
Yes I can take a look :) |
I checked the code. It can't be used in react component because we need more operations to do For example
|
If you don't feel comfortable writing a codemod then you don't have to. This is fine to do in reviewable chunks as is any PR. |
I see the PR of @oliviertassinari, maybe I wanted to complicate my life trying to do everything with multiple transformations
|
@marcosvega91 Thanks :) |
}); | ||
|
||
it('converts an rgba color string to an object with `type` and `value` keys', () => { | ||
const { type, values } = decomposeColor('rgba(255, 255, 255, 0.5)'); | ||
assert.strictEqual(type, 'rgba'); | ||
assert.deepEqual(values, [255, 255, 255, 0.5]); | ||
expect(type).to.equal(type, 'rgba'); |
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 take care of this type of regressions in #20799
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.
If you need an hand in something write a message
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.
Definitely, I won't have the time to handle the last standing usage of assert. However, we are close to the finish line :), if we could remove this duplication of solution for the same problem (assert vs expect) in the codebase, it would be really great.
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.
You didn't merge your solution on master branch yet right?
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.
Not yes, waiting for reviews first.
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.
okok
I update all the test with expect instead of assert