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 assertion-error to it's latest major version! #1543

Merged
merged 3 commits into from Oct 21, 2023

Conversation

koddsson
Copy link
Member

assertion-error is now a ES Module.

@koddsson
Copy link
Member Author

@keithamus does this failure look familiar at all?

  !

  0 passing (6ms)
  1 failing

  1) should
       frozen:
     AssertionError: implementation frames not properly filtered from stack trace: expected 'AssertionError: expected {} to be fro…' not to match /at [a-zA-Z]*(Getter|Wrapper|(\.)*assert)/
      at Assertion.assert (file:///Users/cr91ew/src/koddsson/chai/chai.js:1651:11)
      at Proxy.assertMatch (file:///Users/cr91ew/src/koddsson/chai/chai.js:2752:8)
      at Proxy.methodWrapper (file:///Users/cr91ew/src/koddsson/chai/chai.js:1814:25)
      at globalErr (file:///Users/cr91ew/src/koddsson/chai/test/bootstrap/index.js:42:23)
      at Context.<anonymous> (file:///Users/cr91ew/src/koddsson/chai/test/should.js:3267:5)
      at process.processImmediate (node:internal/timers:478:21)

The stacktrace for the error in question looks like this:

AssertionError: expected {} to be frozen
    at Assertion.assert (file:///Users/cr91ew/src/koddsson/chai/chai.js:1651:11)
    at Assertion.<anonymous> (file:///Users/cr91ew/src/koddsson/chai/chai.js:3251:8)
    at Assertion.propertyGetter (file:///Users/cr91ew/src/koddsson/chai/chai.js:1689:29)
    at Reflect.get (<anonymous>)
    at Object.proxyGetter [as get] (file:///Users/cr91ew/src/koddsson/chai/chai.js:1773:22)
    at file:///Users/cr91ew/src/koddsson/chai/test/should.js:3268:24
    at globalErr (file:///Users/cr91ew/src/koddsson/chai/test/bootstrap/index.js:37:5)
    at Context.<anonymous> (file:///Users/cr91ew/src/koddsson/chai/test/should.js:3267:5)
    at callFn (/Users/cr91ew/src/koddsson/chai/node_modules/mocha/lib/runnable.js:366:21)
    at Runnable.run (/Users/cr91ew/src/koddsson/chai/node_modules/mocha/lib/runnable.js:354:5)
    at Runner.runTest (/Users/cr91ew/src/koddsson/chai/node_modules/mocha/lib/runner.js:677:10)
    at /Users/cr91ew/src/koddsson/chai/node_modules/mocha/lib/runner.js:801:12
    at next (/Users/cr91ew/src/koddsson/chai/node_modules/mocha/lib/runner.js:594:14)
    at /Users/cr91ew/src/koddsson/chai/node_modules/mocha/lib/runner.js:604:7
    at next (/Users/cr91ew/src/koddsson/chai/node_modules/mocha/lib/runner.js:486:14)
    at Immediate._onImmediate (/Users/cr91ew/src/koddsson/chai/node_modules/mocha/lib/runner.js:572:5)
    at process.processImmediate (node:internal/timers:478:21) 

@keithamus
Copy link
Member

keithamus commented Oct 18, 2023

yeah AssertionError is failing to properly remove stack frames for some reason. We have code that elides stack trace lines from Chai itself, so you can see your own test stack frames more clearly.

* By default, also validate that the thrown error's stack trace doesn't contain
* Chai implementation frames. Stack trace validation can be disabled by
* providing a truthy `skipStackTest` argument.

@koddsson
Copy link
Member Author

yeah AssertionError is failing to properly remove stack frames for some reason. We have code that elides stack trace lines from Chai itself, so you can see your own test stack frames more clearly.

Ok cool that jives with what I was looking at. Looking at the difference between assertion-error v1 and v2 it wasn't obvious what we changed here. I'll have to give this another look.

@koddsson
Copy link
Member Author

It looks like chai previously treated AssertionError as a object and not a error but now it correctly identifies it as a error so loupe sets the message differently. Do we wanna try to keep the existing output or change the test?

@koddsson
Copy link
Member Author

What do you think @keithamus ?

@koddsson
Copy link
Member Author

Hmm, no this should work 🤔

@koddsson
Copy link
Member Author

Reading the loupe code this is working as intended so I'm gonna update this test.

@koddsson koddsson changed the title update assertion-error Update assertion-error to it's latest major version! Oct 21, 2023
@koddsson koddsson marked this pull request as ready for review October 21, 2023 13:01
@koddsson koddsson requested a review from a team as a code owner October 21, 2023 13:01
Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@koddsson koddsson merged commit 54d3e6b into chaijs:5.x.x Oct 21, 2023
3 checks passed
@koddsson koddsson deleted the update-assertion-error branch October 21, 2023 19:18
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.

None yet

2 participants