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.

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

[CENT-361] Modify TokenTestUtils function buildExpectedState/getActualState #228

Merged
merged 9 commits into from
Oct 24, 2018

Conversation

mirathewhite
Copy link
Contributor

Updated TokenTestUtils to use the Accounts interface for managing token state

  • Create empty state object fiatTokenEmptyState using all Accounts
  • Use fiatTokenEmptyState in buildExpectedState
  • getActualState uses compact lambda expressions

Removed some redundant code from other files.

@mirathewhite mirathewhite changed the title use compact state representation and getters for FiatToken [CENT-361] Modify TokenTestUtils function buildExpectedState/getActualState Oct 17, 2018
@mirathewhite
Copy link
Contributor Author

Unit test run was taking about 1 hour. I removed some accounts from Accounts. I also created a subset of Accounts (allowanceMappingAccounts) for evaluating the allowances map. This decreased the typical unit test time from 6500ms to 2700ms.
-- Some unit tests changed to use different accounts
-- getAccountState now queries a mapping in parallel
-- Removed duplicate accounts
-- Removed some await statements that were causing sequential execution
-- Modified spreadsheets to avoid code/description discrepancies
-- Verifier correctly identifies unit tests suites (contracts) that are not in any spreadsheet

@walkerq
Copy link
Contributor

walkerq commented Oct 19, 2018

I have some concerns with this approach:

  • Testing only a subset of account allowances is faster, but we can't be sure that other allowances are not being modified, so we might as well defensively test all combinations of our known account allowances. Even if the current tests only modify a subset of account allowances, future tests might modify allowances outside of that subset.
  • We should keep the definitions of the expected state objects defined statically. That way, it is easy to verify that we are checking all the defaults we want to check. Building up the object's structure dynamically saves lines of code, but adds risk that we are not setting defaults correctly.

What do you think @o-a-hudson?

@o-a-hudson
Copy link
Contributor

I have some concerns with this approach:

  • Testing only a subset of account allowances is faster, but we can't be sure that other allowances are not being modified, so we might as well defensively test all combinations of our known account allowances. Even if the current tests only modify a subset of account allowances, future tests might modify allowances outside of that subset.
  • We should keep the definitions of the expected state objects defined statically. That way, it is easy to verify that we are checking all the defaults we want to check. Building up the object's structure dynamically saves lines of code, but adds risk that we are not setting defaults correctly.

What do you think @o-a-hudson?

I agree with @walkerq 's suggestions here. I think that will make the testing less error-prone which seems worth it given that this is smart contract code. I think we could consider other approaches for speeding up the tests, such as parallelizing the tests across different blockchain network instances each with a subset of the tests.

@@ -112,11 +111,20 @@ async function checkState(_tokens, _customVars, emptyState, getActualState, acco
// returns an object containing the results of calling mappingQuery on each account
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: mappingQuery -> accountQuery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

await expectRevert(token.mint(Accounts.pauserAccount, mintAmount, { from: Accounts.arbitraryAccount }));
//await expectRevert(token.mint(Accounts.pauserAccount, 0, { from: Accounts.arbitraryAccount }));
await expectRevert(token.mint(Accounts.arbitraryAccount2, mintAmount, { from: Accounts.arbitraryAccount }));
//await expectRevert(token.mint(Accounts.arbitraryAccount2, 0, { from: Accounts.arbitraryAccount }));
Copy link
Contributor

Choose a reason for hiding this comment

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

comment can be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

test/TokenTestUtils.js Show resolved Hide resolved
test/TokenTestUtils.js Show resolved Hide resolved
@@ -0,0 +1,140 @@
1) Contract: FiatToken_MiscTests
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to commit this file, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops - this is a personal scratch file.

};


function recursiveSetAccountDefault(accounts, value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to move this to AccountUtils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@mirathewhite mirathewhite merged commit dc693a0 into circlefin:multi-issuer Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants