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
Branch
herald-refactor-stack
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23084
Build 45788: Build Diffecash-herald-tests
Build 45787: arc lint + arc unit

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

just return await telegramBot.sendMessage(...)

88

that's unrelated

89

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

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

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

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