Page MenuHomePhabricator

[e.cash] Add functions to fetch exchanges and score and sort them
ClosedPublic

Authored by johnkuney on Jun 14 2023, 16:13.

Details

Summary

Function to fetch scorecard.cash exchange list
Function to score the responses based on some added scoring criteria
Function to make sure the sorted list is divisible by three
Test for all the functions

Test Plan

npm run test
see if all test pass

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ecash-get-scores
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 24038
Build 47686: Build Diff
Build 47685: arc lint + arc unit

Event Timeline

bytesofman added a subscriber: bytesofman.
bytesofman added inline comments.
web/e.cash/data/__tests__/scores.test.js
59 ↗(On Diff #40788)

Inside describe('getScoreCardData', () => {,

add a beforeEach block and include global.fetch = jest.fn(); in there

add an afterEach block and include

global.fetch.mockClear();
        delete global.fetch;

in there

useful to avoid mistakes like below, which can be a massive headache to discover (say you add new unit tests later on, happen to mock fetch, in some totally different file. they all fail for reasons that don't make any sense in the logs because your last mock in this file didn't delete global.fetch even though the others did

104 ↗(On Diff #40788)

no delete global.fetch here

still "works" but only bc no other files with unit tests using this mock.

web/e.cash/data/scores.js
113 ↗(On Diff #40788)

scoringCriteria

117 ↗(On Diff #40788)

in general, best to avoid _ in variable names

scoring_criteria should be scoringCriteria

exceptions for constants, though JS convention there would be to also keep it all in block caps, e.g. SCORING_CRITERIA -- which, anyway, this isn't that kind of a constant

117 ↗(On Diff #40788)

This function is doing two things

  1. determine score based on a certain critera
  2. sort exchanges alphabetically by name, then by lowest confirmation requirements, then by score

Need to at least change the name of the function to calculateScoreAndSortExchanges ... this is kind of awkward though, there's not really a good reason why both of these things should happen in the same function. Complicates review and unit tests.

You should probably split this up to be two functions, getScore and sortExchanges -- each with unit tests

This revision now requires changes to proceed.Jun 14 2023, 16:43

break out the sort function from the score function

bytesofman added inline comments.
web/e.cash/data/__tests__/scores.test.js
46
This revision now requires changes to proceed.Jun 14 2023, 19:57
Fabien added a subscriber: Fabien.
Fabien added inline comments.
web/e.cash/data/__tests__/scores.test.js
70 ↗(On Diff #40795)

Please also check with an array of soze > 3 (e.g 8)

add another test to makedivisiblebythree to check > 3

This revision is now accepted and ready to land.Jun 15 2023, 17:21