Page MenuHomePhabricator

[Cashtab][Alias] Update SendToken.js to use api endpoint in place of getAddressFromAlias
ClosedPublic

Authored by emack on Jul 14 2023, 14:46.

Details

Summary

T3216

Update the alias lookup in sendToken.js component's address input field to use the alias api endpoint

Test Plan
  • npm test
  • change the alias flag in Ticker.js to true
  • npm start
  • navigate to the Send Token page and click into any token
  • input a pre-registered alias in the address input field and ensure it parses to the right address
  • ensure the hyperlink underneath links correctly to the explorer page for this address
  • send a token amount to this alias and ensure successful broadcast, and sanity check tx details in explorer
  • input a valid eCash address and ensure it is accepted
  • input a valid prefix-less eCash address and ensure it is accepted
  • input an invalid eCash address and invalid alias and ensure it triggers validation error
  • input an unregistered alias and ensure the eCash Alias does not exist or yet to receive 1 confirmation validation error is displayed

Diff Detail

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

Event Timeline

emack requested review of this revision.Jul 14 2023, 14:46
Fabien requested changes to this revision.Jul 17 2023, 09:09
Fabien added a subscriber: Fabien.

Why is there no test change ?

cashtab/src/components/Send/SendToken.js
276 ↗(On Diff #41448)

Is this function still in use ?

278 ↗(On Diff #41448)

There is no need for the cache anymore ?

294 ↗(On Diff #41448)

What does this do ? Display a popup window ?

This revision now requires changes to proceed.Jul 17 2023, 09:09
emack marked 3 inline comments as done.EditedJul 17 2023, 12:43

Why is there no test change ?

When pushing this diff the aliasEnabled flag in Ticker.js is set to false, so that landing this diff does not prematurely publish this particular alias UI function. If the flag is enabled to true, then npm test will pick up on the UI changes and result in updates to the snapshots for this component.
What I'll do is add some additional unit tests that checks the state of this component.

cashtab/src/components/Send/SendToken.js
276 ↗(On Diff #41448)

This function is being deprecated after this diff (T3216)

278 ↗(On Diff #41448)

Yup, alias cache deprecation is planned as one of the subsequent diffs (T3216)

294 ↗(On Diff #41448)

yup

image.png (164×336 px, 14 KB)

emack marked 3 inline comments as done.

Added missing unit test to verify component state at initialization

Fabien requested changes to this revision.Jul 18 2023, 14:25

There is no test for the error cases

This revision now requires changes to proceed.Jul 18 2023, 14:25

Failed tests logs:

====== CashTab Unit Tests:  Verify the SendToken component throws error on missing Wallet object ======
Error: expect(received).toBe(expected) // Object.is equality

Expected: "Cannot destructure property 'wallet' of '((cov_1erx5eord5(...).s[16]++) , _react.default.useContext(...))' as it is undefined."
Received: "Cannot destructure property 'wallet' of '((cov_2xkkrl9h1(...).s[16]++) , _react.default.useContext(...))' as it is undefined."
    at Object.<anonymous> (/work/cashtab/src/components/Send/__tests__/SendToken.test.js:134:34)
    at Promise.then.completed (/work/cashtab/node_modules/jest-circus/build/utils.js:391:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/work/cashtab/node_modules/jest-circus/build/utils.js:316:10)
    at _callCircusTest (/work/cashtab/node_modules/jest-circus/build/run.js:218:40)
    at _runTest (/work/cashtab/node_modules/jest-circus/build/run.js:155:3)
    at _runTestsForDescribeBlock (/work/cashtab/node_modules/jest-circus/build/run.js:66:9)
    at run (/work/cashtab/node_modules/jest-circus/build/run.js:25:3)
    at runAndTransformResultsToJestFormat (/work/cashtab/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:170:21)
    at jestAdapter (/work/cashtab/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:82:19)
    at runTestInternal (/work/cashtab/node_modules/jest-runner/build/runTest.js:389:16)
    at runTest (/work/cashtab/node_modules/jest-runner/build/runTest.js:475:34)
    at Object.worker (/work/cashtab/node_modules/jest-runner/build/testWorker.js:133:12)

Each failure log is accessible here:
CashTab Unit Tests: Verify the SendToken component throws error on missing Wallet object

emack planned changes to this revision.Jul 18 2023, 16:02

React/Jest dynamically generates the ((cov_1erx5eord5(...).s[16]++) portion of the expected error message Cannot destructure property 'wallet' of ((cov_1erx5eord5(...).s[16]++) _react.default.useContext(...)) as it is undefined. at the time of test execution, hence updating the error test case to check that the error message string contains Cannot destructure property 'wallet' as that is the core aspect being validated

cashtab/src/components/Send/SendToken.js
283 ↗(On Diff #41507)

I was talking about this one :) Maybe there is a test already but I couldn't find it ?

emack planned changes to this revision.Jul 20 2023, 23:29

Using unit tests to simulate frontend alias input into the address field, mocking the API response then validating the subsequent frontend error is proving to be a bit of a head scratcher, even with the react testing library. Will keep chipping away at this. So far the screen and renderhook approaches have not yielded any results.

That's because you are not unit testing the feature. Your input is a giant handler, if you cut it into pieces that becomes much more manageable. You don't want to test user input or the query or the UI rendering, but the logic in between: if the requested alias doesn't exist, it doesn't return an address but an error.

emack marked an inline comment as done.EditedJul 22 2023, 01:38

That's because you are not unit testing the feature. Your input is a giant handler, if you cut it into pieces that becomes much more manageable. You don't want to test user input or the query or the UI rendering, but the logic in between: if the requested alias doesn't exist, it doesn't return an address but an error.

In that case there's already a unit test in aliasUtils.test.js that tests the case of an alias that isn't registered.
https://github.com/Bitcoin-ABC/bitcoin-abc/blob/c6b2bf9f979ba38e3d6ffd41d1257c6d0a3dda1e/cashtab/src/utils/__tests__/aliasUtils.test.js#L194-L212
But that is specifically validating the endpoint response is handled though. If you want to see automated testing of the UI throwing the error message above based on this endpoint response then that's the tricky part with React frontend testing where you have to simulate from the point of user input + mock the endpoint response + somehow catching the simulated UI response.
We've been manually testing these aspects thus far and it's definitely something we should be automating but may need a refactor of how the app and its various components are structured.

^ @Fabien thoughts on comment above and whether we can explore this in a separate diff?

I'm fine with running integration tests manually for now, at least it doesn't belong to this diff for sure

emack requested review of this revision.Jul 28 2023, 15:34
This revision is now accepted and ready to land.Jul 28 2023, 20:15