-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JWWEB-687 Add tests for merge assets function #1119
base: feature/cicd-tests
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! But it needs some improvements.
@@ -0,0 +1,26 @@ | |||
// @flow strict | |||
|
|||
import addETHAsset from '../addETHAsset' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to import from index.js
because it is the public interface you are testing for existence.
expect(eth.blockchainParams).toBeDefined() | ||
expect(eth.blockchainParams.address).toEqual(ETH_ADDRESS) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also tests required:
- non-empty assets list
- with
ethereum
already in assets list - with mock for
getAssetsMainnet
function returning various results (empty, non-empty, withethereum
)
const jnt: ?DigitalAsset = result[JNT_ADDRESS] | ||
|
||
if (!jnt) { | ||
throw new Error('JNT asset not found') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to throw here.
First, you are testing jnt
existence in next assertion.
Second, if test continues execution on assertion not passed, you could use safe assertions after, for example: https://jestjs.io/docs/en/expect#tohavepropertykeypath-value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const eth: ?DigitalAsset = result[ETH_ADDRESS]
means that eth
can be null
or undefined
.
expect(eth).toBeDefined()
will not throw if eth
is null
.
const azbit: ?DigitalAsset = result[AZBIT_ADDRESS_CS] | ||
|
||
if (!azbit) { | ||
throw new Error('Azbit asset not found') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
const azbit: ?DigitalAsset = result[AZBIT_ADDRESS_CS] | ||
|
||
if (!azbit) { | ||
throw new Error('Azbit asset not found') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
const jnt: ?DigitalAsset = result[JNT_ADDRESS] | ||
|
||
if (!jnt) { | ||
throw new Error('JNT asset not found') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
No description provided.