Page MenuHomePhabricator

[alias-server] Removing untested logic from parseWebsocketMessage
ClosedPublic

Authored by bytesofman on Apr 4 2023, 20:37.

Details

Summary

T3060

Depends on D13566

In order to simplify future reviews, deprecate untested logic from parseWebsocketMessage. Replace with commented TODO statements. Add basic unit tests for the function.

Unused functions and mocks are removed from utils.js. Functions that remain and are not used by the app -- these will have unit tests added one at a time in future diffs, as the added app logic requires.

Test Plan

Review simplified logic in parseWebsocketMessage. npm test and all pass

Diff Detail

Repository
rABC Bitcoin ABC
Branch
alias-server-spring-cleaning
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 22991
Build 45602: Build Diffalias-server-tests
Build 45601: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Apr 5 2023, 08:59
Fabien added a subscriber: Fabien.
Fabien added inline comments.
web/alias-server/src/websocket.js
34 ↗(On Diff #39247)

If you need to change the behavior for the unit tests, your test is not relevant

54 ↗(On Diff #39247)

This is not a loop

75 ↗(On Diff #39247)

the if() return else() return antipattern

This revision now requires changes to proceed.Apr 5 2023, 08:59

fix if/else antipattern and replace 'loop' with function name

bytesofman added inline comments.
web/alias-server/src/websocket.js
34 ↗(On Diff #39247)

I'm not able to mock async chronik calls that will later come in this function.

gating them with this flag and accepting mocks of known chronik responses is an effective way to test function logic without hitting the API

Alternatively, the unit test can hit the API.

Another issue that will be raised is creating / reading / writing to mongod. There are complicated modules available which mock this, but then we have a similar issue; unit test is testing a mock module and not the used database.

Available options imo are

  1. Gate with isUnitTest variable to test function behavior at key logic junctures with templated API and mongod responses
  2. Call the APIs + fire up a database before unit tests

In this case I'm just used to avoiding API calls in unit tests, hence the flag. But if we can do these in CI, might as well.

Okay, for now I have removed all the functions from parseWebsocketMessage and refactored the unit tests.

The functions removed will need to be refactored to accept chronik as a param. Future diffs will refactor for this case and add a mock chronik to the unit tests.

web/alias-server/src/websocket.js
86 ↗(On Diff #39278)

that would crash on app startup since wsMsg.blockHash is undefined

This revision is now accepted and ready to land.Apr 5 2023, 13:00
This revision was landed with ongoing or failed builds.Apr 5 2023, 13:26
This revision was automatically updated to reflect the committed changes.