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

Update sinon-chai to use latest versions of chai and sinon #162

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

neverbot
Copy link

@neverbot neverbot commented Jan 25, 2024

I'm trying to update this package to be used with the new version of chai (from v4 to v5), and sinon (from v9 to v17). chai is now an ESM module, so this package must be migrated to ESM too. This would solve #160.

Important

There is a different PR from @robinbisping (#161) that updates "just" chai and I think works fine, should be accepted first. This one tries to update both chai and sinon and it's not working yet.

Done:

  • Dependencies updated.
  • Update to ESM modules, change require to import/export.

Need reviewing:

  • sinon-chai used the UMD pattern to allow using it with AMD, CommonJS and browser. With ESM modules this is (i think) no longer necessary, just an ESM module with export... but I'm a backend programmer, so I need somebody to test this.
  • sinon has been updated from v9 to v17... some things have changed, and the information messages shown to the user are different. I've changed some of the "expected foo but bar" messages to better match the current sinon styles... and the existing tests to fit those new messages. Needs review.

Need to fix:
There are 5 tests which i'm not able to fix:

  • 2 tests about call order showing the function names
       -expected spyA to have been called before [Function]
       +expected spyA to have been called before function spyB() {}
    
  • 3 tests about call arguments with match
       -expected spy to not have been called with match { test: [Function], message: 'match(1)' }
       +expected spy to not have been called with match
       +1 match(1)
    

I don't know why sinon tries to serialize some objects instead of returning just a string with the right message. Don't know enough about the inner workings of sinon to understand what is happening. Somebody wants to help?

@koddsson
Copy link
Member

Hey @neverbot; sorry for the very late reply. Could you resolve the conflicts here if you are still interested in landing this PR?

@neverbot
Copy link
Author

Hey @neverbot; sorry for the very late reply. Could you resolve the conflicts here if you are still interested in landing this PR?

Hi @koddsson, this PR was not 100% working, when I tried to update the sinon version, some of the error messages returned from sinon changed, and I have not enough knowledge about this to know if it was my fault or just some inner changes in sinon.

I've seen you've already used ESM modules, which was half the PR, kudos!

If somebody else (with more knowledge about sinon than me) wants to cherry-pick some commit from here, feel free to do it. Either case, we could close the PR.

@43081j
Copy link
Contributor

43081j commented May 13, 2024

im happy to sort it out unless you want to @koddsson

let me know

@koddsson
Copy link
Member

@43081j Sounds good :)

@neverbot Sorry again for the very late reply. We are really appreciative of you taking the time to work on this <3

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.

3 participants