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

Create custom methods to replace Sinon mocks #1406

Open
paulmillr opened this issue Jan 23, 2025 · 11 comments
Open

Create custom methods to replace Sinon mocks #1406

paulmillr opened this issue Jan 23, 2025 · 11 comments

Comments

@paulmillr
Copy link
Owner

We're using Sinon but I think the functionality can be easily replaced by our own methods.

Maybe we can make aspy less fragile? cc @43081j

@43081j
Copy link
Collaborator

43081j commented Jan 23, 2025

we're mostly using it for the sinon-chai assertions (v.should.have.been.called etc) i think

i'd opt to replace it with tinyspy tbh. i don't think we need to build our own call tracking code etc and that package is tiny compared to sinon

@paulmillr
Copy link
Owner Author

sounds good

v.should.have.been.called

can be replaced by a custom shortcut

@43081j
Copy link
Collaborator

43081j commented Jan 23, 2025

If we use tinyspy, we should probably switch away from should

So we do assert.ok(spy.called) for example. Or the expect equivalent if we prefer that style of assertion

@paulmillr
Copy link
Owner Author

we should probably switch away from should

if you're talking about chai, I agree

@paulmillr
Copy link
Owner Author

node:assert has everything we need

@43081j
Copy link
Collaborator

43081j commented Jan 23, 2025

I saw you moved away from it when adding bun

But it looks like bun should support it. So maybe we should:

  • bring node:assert back
  • remove micro should
  • remove chai
  • remove sinon and sinon-chai
  • add tinyspy

I can have a go and see what I end up with

@paulmillr
Copy link
Owner Author

remove micro should

No. Why? Bun, deno don't support node:test.

@paulmillr
Copy link
Owner Author

node:test != node:assert

@43081j
Copy link
Collaborator

43081j commented Jan 23, 2025

Ah sorry I misread the source

I understand they're not the same. It was just me misreading

I thought we pulled assert from node already and didn't realise we use chai

I'll have a go at what I said but keep the test runner

@paulmillr
Copy link
Owner Author

paulmillr commented Jan 27, 2025

@43081j great job. A few things:

waitFor signature is a huge mess. We need something which only accepts 1-3 arguments. Not array of arrays etc.

await waitFor([spy]);
// =>
await waitFor(spy);

await waitFor([[spy, 1, [expectedPath]]]);
// =>
await waitFor(spy, 1, expectedPath)
  • ok(calledWith()) seems like it could be replaced by just calledWith if calledWith would auto-throw an error?
  • calledWith accepts array if arguments. is it necessary? Maybe just ...args?

@43081j
Copy link
Collaborator

43081j commented Jan 27, 2025

I agree, I don't like the shape of waitFor in general

  • calledWith can be an assertion. I actually had it that way locally but wasn't sure if to push it
  • we could use spread, I don't have a preference so am happy to change it

I'll make a branch for these things at some point

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

No branches or pull requests

2 participants