[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 :) |
oliviertassinari
changed the title
| }); | ||
|
|
||
| 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.
I take care of this type of regressions in #20799
There was a problem hiding this comment.
If you need an hand in something write a message
There was a problem hiding this comment.
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.
You didn't merge your solution on master branch yet right?
There was a problem hiding this comment.
Not yes, waiting for reviews first.
I update all the test with expect instead of assert