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

fix(tests): restructure mockApi tests to integrate with historical api #702

Merged
merged 37 commits into from
Oct 12, 2021

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Oct 8, 2021

This is part 2 of: #699

The purpose of this PR is to restructure the mockApi, and create an adjacent mockHistoricalApi. Since we are removing all the query...at() calls we need to reflect that in the tests. I am taking this time to also reorganize our mockApis and isolate the tests even more. None of the unit tests themselves are being changed, just the structure, and organization.

Another benefit of this PR will be that when its time to transition a Service's structure for querying historical blocks in this Tracking Issue #698

Services:

  • BlocksService.spec.ts
  • BlocksTraceService.spec.ts
  • AccountsAssetsService.spec.ts
  • AccountsBalanceInfoService.spec.ts
  • AccountsStakingInfoService.spec.ts
  • AccountsVestingInfoService.spec.ts
  • NodeNetworkService.spec.ts
  • NodeTransactionPoolService.spec.ts
  • NodeVersionService.spec.ts
  • PalletsAssetsService.spec.ts
  • PalletsStakingProgressService.spec.ts
  • PalletsStorageService.spec.ts
  • ParasService.spec.ts
  • RuntimeCodeService.spec.ts
  • RuntimeMetadataService.spec.ts
  • RuntimeSpecService.spec.ts
  • TransactionFeeEstimateService.spec.ts
  • TransactionMaterialService.spec.ts
  • TransactionSubmitService.spec.ts

Extras

  • Cleanup MockApi

@TarikGul TarikGul marked this pull request as ready for review October 8, 2021 23:05
Copy link
Contributor

@insipx insipx left a comment

Choose a reason for hiding this comment

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

LGTM, mostly just mock objects for the tests(if i read through it correctly), which pass. Looks like solid improvement

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Looks sane to me.

...defaultMockApi,
...queryMockApi,
at: (_hash: Hash) => mockHistoricalApi,
} as unknown as ApiPromise;
Copy link
Contributor

Choose a reason for hiding this comment

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

dq: why is it necessary to cast to unknown before casting to ApiPromise?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because the typescript compiler will have issues when converting or typecasting the original type to the newly assigned one (almost like a merge conflict). Therefore, unknown allows us to give the object a clean slate before assigning it a new type.

src/services/accounts/AccountsBalanceInfoService.spec.ts Outdated Show resolved Hide resolved
const epochIndexAt = (_hash: Hash) =>
Promise.resolve().then(() => polkadotRegistry.createType('u64', 330));

const genesisSlotAt = (_hash: Hash) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a "slot" in this context?

Copy link
Member Author

Choose a reason for hiding this comment

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

The slot at which the first epoch actually started. This is 0
until the first block of the chain.

This slot is not to be confused with the parachain slot. It has to do with the BABE block production mechanism.
An epoch is a contiguous number of slots (From the substrate docs)

@TarikGul TarikGul merged commit 2bf71ad into master Oct 12, 2021
@TarikGul TarikGul deleted the tarik-update-tests branch October 12, 2021 12:51
This pull request was closed.
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