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

Replace sinon with nise #554

Closed
HarelM opened this issue Oct 21, 2021 · 4 comments · Fixed by #930
Closed

Replace sinon with nise #554

HarelM opened this issue Oct 21, 2021 · 4 comments · Fixed by #930

Comments

@HarelM
Copy link
Collaborator

HarelM commented Oct 21, 2021

Motivation

It seems that we are using sinon mainly for the fake http server.
nise is this part of sinon exactly and their typings is a bit better than sinon's.
I would advise to add nise to the jest test migration effort and remove sinon once we are done with the migration.

@astridx
Copy link
Contributor

astridx commented Oct 22, 2021

What do you mean exactly? Would you like to change now or should we add it to list #458 ?

@HarelM
Copy link
Collaborator Author

HarelM commented Oct 22, 2021

We can do it whenever we want - either when we finish migrating all the tests and then do a sweep to change it, or change it now and make sure that every test that we convert uses the "new way".
I'm fine with both...
We can add it to #458, but it's not something that we introduced as part of the migration but rather an improvement I think is needed and I found out about it while doing the migration.
It's not that critical, but it might help with the migration if we do it now...

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 2, 2022

@birkskyum thanks for pushing this forward!!

@birkskyum
Copy link
Member

Sure! I'm still trying to figure out how we can get the remaining integration tests modernized too and leaved test'em etc. behind for good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants