Page MenuHomePhabricator

[herald] Unit tests for parseWebsocketMessage
ClosedPublic

Authored by bytesofman on Apr 7 2023, 17:33.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC3b3010946eda: [herald] Unit tests for parseWebsocketMessage
Summary

T2972

Depends on D13616

Add unit tests for parseWebsocketMessage

These unit tests required a mock Telegram bot and improvements to chronikMock.js

For now, I think it's okay to maintain chronikMock.js as distinct files in both alias-server and ecash-herald. The incremental work in both apps is a good way to learn what features this mock needs. It will eventually be extended to Cashtab and live as a centralized dependency.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Clean up comments in telegramBotMock.js

Check if telegramBot called or did not call expected routine

Fabien requested changes to this revision.Apr 9 2023, 06:42
Fabien added a subscriber: Fabien.
Fabien added inline comments.
apps/ecash-herald/src/websocket.js
79 ↗(On Diff #39409)

just return await telegramBot.sendMessage(...)

88 ↗(On Diff #39409)

that's unrelated

89 ↗(On Diff #39409)

you're missing a return false, either here or at the end of the function (then you don't need it in the default: case)

apps/ecash-herald/test/mocks/telegramBotMock.js
6 ↗(On Diff #39409)

It's not a good idea to rely on a sample file, people typically delete/rename these. Can't you just hardcode some default dummy values in the mock?

apps/ecash-herald/test/websocketTests.js
158 ↗(On Diff #39409)

You are missing a test case: returns false if send_message failed

This revision now requires changes to proceed.Apr 9 2023, 06:42
bytesofman marked an inline comment as done.

improve telegrambot mock, allow it to throw errors, code improvements from feedback

apps/ecash-herald/src/websocket.js
88 ↗(On Diff #39409)

These cases are dropped to match the scope of the newly introduced unit test.

Fabien added inline comments.
apps/ecash-herald/test/websocketTests.js
197 ↗(On Diff #39493)

That assert is not checking anything useful, you can remove it

This revision is now accepted and ready to land.Apr 10 2023, 18:51