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

test: switch from should style to expect #1408

Merged
merged 2 commits into from
Jan 25, 2025
Merged

test: switch from should style to expect #1408

merged 2 commits into from
Jan 25, 2025

Conversation

43081j
Copy link
Collaborator

@43081j 43081j commented Jan 24, 2025

Switches from using should assertions (e.g. foo.should.equal(303)) to expect assertions (e.g. expect(foo).to.equal(303)).

This is a stepping stone to moving off sinon and possibly chai in future.

FYI this was automated using an ast-grep codemod found here (which itself has tests)

Switches from using `should` assertions (e.g. `foo.should.equal(303)`)
to `expect` assertions (e.g. `expect(foo).to.equal(303)`).

This is a stepping stone to moving off `sinon` and possibly `chai` in
future.
@43081j
Copy link
Collaborator Author

43081j commented Jan 24, 2025

just found why one of the tests was flaky too!

it was waiting for 3 events instead of 5... so fixed it while i was in there

@43081j 43081j requested a review from paulmillr January 24, 2025 15:18
@paulmillr
Copy link
Owner

Why don’t we just move to assert? Or maybe assert has expect inside?

@paulmillr
Copy link
Owner

I am not a huge fan of assert, but at least it is everywhere

@43081j
Copy link
Collaborator Author

43081j commented Jan 24, 2025

We are moving to assert, I'm just not doing it in one massive PR as it'll be difficult to review

  1. Move from should to expect so all assertions are consistent (we use a mixture right now)
  2. Move from expect to assert (node:assert) (test: migrate from chai to node:assert #1409)
  3. Move from sinon to tinyspy

Roughly this is my plan

43081j added a commit that referenced this pull request Jan 24, 2025
Migrates away from chai's `expect` assertions to `node:assert`.

Depends on #1408
@43081j
Copy link
Collaborator Author

43081j commented Jan 24, 2025

i've opened #1409 which we can aim to merge once this PR lands

then we will have moved to node:assert fully

@paulmillr paulmillr merged commit 09ed5b2 into main Jan 25, 2025
21 of 22 checks passed
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 this pull request may close these issues.

2 participants