Skip to content

Fix the has an onerror callback websocket test#135

Merged
dentarg merged 1 commit intocloudamqp:mainfrom
dentarg:test-browser/websocket/onerror-fix
Apr 5, 2025
Merged

Fix the has an onerror callback websocket test#135
dentarg merged 1 commit intocloudamqp:mainfrom
dentarg:test-browser/websocket/onerror-fix

Conversation

@dentarg
Copy link
Copy Markdown
Member

@dentarg dentarg commented Apr 5, 2025

This is how the test in test/test.ts looks.

amqp-client.js/test/test.ts

Lines 596 to 605 in 48a76b2

test("has an onerror callback", async () => {
const amqp = getNewClient()
const conn = await amqp.connect()
const ch = await conn.channel()
let errMessage: string | null = null
ch.onerror = vi.fn((reason) => errMessage = reason)
await expect(ch.exchangeDeclare("none", "none")).rejects.toThrow()
expect(ch.onerror).toBeCalled()
expect(errMessage).toMatch(/invalid exchange type/)
})

I think this is the intention. Right @ngbrown?

Somehow we missed it in #71

Before this change, the test failed like this

 FAIL  |test-browser| test-browser/websocket.ts > has an onerror callback
AssertionError: expected "spy" to be called at least once
 ❯ Proxy.methodWrapper ../../../../node_modules/.vite/deps/vitest___chai.js:1050:29
 ❯ test-browser/websocket.ts:600:23
    598|   conn.onerror = vi.fn((err) => errMessage = err.message)
    599|   await expect(ch.exchangeDeclare("none", "none")).rejects.toThrow()
    600|   expect(conn.onerror).toBeCalled()
       |                       ^
    601|   expect(errMessage).toMatch(/invalid exchange type/)
    602| })

@dentarg
Fix the has an onerror callback websocket test
b1b194a 
This is how the test in test/test.ts looks.
@dentarg dentarg force-pushed the test-browser/websocket/onerror-fix branch from ff039f2 to b1b194a Compare April 5, 2025 22:58
@dentarg
Copy link
Copy Markdown
Member Author

dentarg commented Apr 5, 2025

I'll merge this, as CI is green now and that makes the situation much better (we can spot other regressions and CI errors).

@dentarg dentarg merged commit cc8448a into cloudamqp:main Apr 5, 2025
5 checks passed
@dentarg dentarg deleted the test-browser/websocket/onerror-fix branch April 5, 2025 23:05
@ngbrown
Copy link
Copy Markdown
Contributor

ngbrown commented Apr 9, 2025

@dentarg Yes, the change looks fine.

I was copying test/test.ts as it was at the time.

It was #58, which merged after #71, that had the change to use ch.onerror in test/test.ts but didn't update test-browser/websocket.ts.

@dentarg
Copy link
Copy Markdown
Member Author

dentarg commented Apr 10, 2025 •
edited
Loading

I see, thanks for explaining it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dentarg @ngbrown