razorpay / blade

Design System that powers Razorpay
https://blade.razorpay.com
MIT License
483 stars 124 forks source link

Relook at all the console mocks in test files #930

Closed kamleshchandnani closed 1 year ago

kamleshchandnani commented 1 year ago

Ref https://github.com/razorpay/blade/pull/928#discussion_r1058850148

Right now we have mocked console errors on top of almost every test file. While that prevents us from filling up our console unnecessarily where we test negative test cases using toThrow, the downside is that in doing so we might miss out on sometimes genuine essential errors.

My proposal would be to add the console mocks to the test suite where required and remove them from the top of all the files

ashutosh887 commented 1 year ago

I would like to work on this @kamleshchandnani

kamleshchandnani commented 1 year ago

@ashutosh887 That's great 🎉 . Please feel free to open a PR.

ashutosh887 commented 1 year ago

Thanks @kamleshchandnani Please guide me how should I proceed!

divyanshu013 commented 1 year ago

You can get started by looking at the contributing guidelines and setup the project. Post that run the test command (from package.json).

Once this setup is operational you'll have to go through all test files that have console mocks (ref original comment).

You can post here if you've doubts about some test module based on your findings.

ashutosh887 commented 1 year ago

Thanks a lot @divyanshu013 . I'll start immediately as per your instructions.

ashutosh887 commented 1 year ago

@kamleshchandnani @divyanshu013 I'm getting this on running yarn in packages/blade -> 'rm' is not recognized as an internal or external command

snitin315 commented 1 year ago

@ashutosh887 Are you using windows? No team member uses it, so it's challenging to provide official support.

We won't be against merging fixes that improve the developer experience on Windows (as long as it doesn't negatively impact the experience on MacOS/Linux).

We will need to update our scripts for compatibility with windows, for example:

https://github.com/razorpay/blade/blob/c73a9cd9216b46903ed100947f0e2c96cdda0584/packages/blade/package.json#L49

rm should be replaced with del-cli or rimraf

and https://github.com/razorpay/blade/blob/c73a9cd9216b46903ed100947f0e2c96cdda0584/packages/blade/package.json#L60

FRAMEWORK=REACT should be replaced with cross-env FRAMEWORK=REACT using the cross-env package.

// @divyanshu013 @kamleshchandnani

divyanshu013 commented 1 year ago

I can recommend setting up WSL on windows, should be closer to our dev envrionment.

ashutosh887 commented 1 year ago

Sure @divyanshu013 Sir. Working on it.

ashutosh887 commented 1 year ago

yarn test keeps on running (10 Minutes+) and shows a lot of errors now:

Sharing one final snippet: Test Suites: 6 failed, 27 passed, 33 of 357 total Tests: 13 failed, 393 passed, 406 total Snapshots: 1 failed, 208 passed, 209 total

// @divyanshu013 @kamleshchandnani @snitin315

ashutosh887 commented 1 year ago

Please have a look team!

divyanshu013 commented 1 year ago

This works for all of us 🤔

Could you try to debug please? Not much info from the shared logs to help narrow this down