Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Problem: missing property-base testing (fix #857) #862

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Problem: missing property-base testing (fix #857) #862

wants to merge 4 commits into from

Conversation

leejw51crypto
Copy link
Contributor

add fast-check testing

tomtau
tomtau reviewed Dec 6, 2021
View reviewed changes
src/service/signers/LedgerSigner.spec.ts
import sdk from '@crypto-org-chain/chain-jslib';
import { Units, Secp256k1KeyPair } from '../../utils/ChainJsLib';

test('should declare coin in ledger correctly', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the property here? cro.Coin not throwing an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whether coin can be created to the maximum, that will not throw any exception

tomtau
tomtau reviewed Dec 6, 2021
View reviewed changes
src/service/signers/TransactionSigner.spec.ts
},
phrase,
);
expect(transferTxSignedHex !== '').to.eq(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also check transferTxSignedHex can be parsed into a transaction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.json

@crypto-matto
Copy link
Collaborator

There are also some eslint error in CI to be fixed as well

 src/service/signers/LedgerSigner.spec.ts:2:10
  ✖   2:10  expect is defined but never used.               @typescript-eslint/no-unused-vars
  ✖   4:10  Big is defined but never used.                  @typescript-eslint/no-unused-vars
  ✖   7:17  Secp256k1KeyPair is defined but never used.     @typescript-eslint/no-unused-vars
  ✖  13:15  networkFee is assigned a value but never used.  @typescript-eslint/no-unused-vars
  ✖  15:13  fee is assigned a value but never used.         @typescript-eslint/no-unused-vars

  src/service/signers/TransactionSigner.spec.ts:4:10
  ✖   4:10  Big is defined but never used.                  @typescript-eslint/no-unused-vars

@leejw51crypto
Copy link
Contributor Author

fixing lint issue

@leejw51crypto leejw51crypto force-pushed the feature/857 branch 2 times, most recently from 9f199ae to 5c6a7fb Compare December 9, 2021 11:25
crypto-matto and others added 3 commits December 9, 2021 22:03
@crypto-matto @leejw51crypto
Update CHANGELOG
febb8c0
@crypto-matto @leejw51crypto
Update CHANGELOG
eb85272
@leejw51crypto
Problem: missing property-base testing (fix crypto-com#857)
36a7219 
add fast-check testing

add tx rlp decoding

tidy up

fix lint and tidy up

fix lint

remove else
ghost
ghost approved these changes Jul 28, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked

Frankramos181422
Frankramos181422 approved these changes Oct 24, 2022
View reviewed changes
src/service/signers/TransactionSigner.spec.ts
);
const decodedTransaction = RLP.decode(transferTxSignedHex);
expect(transferTxSignedHex !== '').to.eq(true);
expect(BigInt(decodedTransaction[4]) === amount).to.eq(true);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • [ ]

src/service/signers/TransactionSigner.spec.ts
},
phrase,
);
expect(transferTxSignedHex !== '').to.eq(true);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.json

src/service/signers/TransactionSigner.spec.ts
},
phrase,
);
const decodedTransaction = RLP.decode(transferTxSignedHex);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • [ ]

src/service/signers/TransactionSigner.spec.ts
phrase,
);
const decodedTransaction = RLP.decode(transferTxSignedHex);
expect(transferTxSignedHex !== '').to.eq(true);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • (True)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomtau tomtau tomtau left review comments

@Frankramos181422 Frankramos181422 Frankramos181422 approved these changes

@calvinaco calvinaco Awaiting requested review from calvinaco

@crypto-matto crypto-matto Awaiting requested review from crypto-matto

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@leejw51crypto @crypto-matto @tomtau @Frankramos181422 @cdc-Hitesh