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

Tests V2 #1026

Merged
merged 40 commits into from
Sep 26, 2024
Merged

Tests V2 #1026

merged 40 commits into from
Sep 26, 2024

Conversation

shunjizhan
Copy link
Collaborator

@shunjizhan shunjizhan commented Sep 21, 2024

Change

Core Updates

  • finally we have test coverage!
  • replaced mandala node with chopsticks acala fork as the test node

Cleanups and Refactors

  • removed a lot of docker stuff in favor for local setups, which improved stability and reduced complexity
  • removed submodules of { hardhat, truffle }-tutorials, and instead use local e2e-{ hardhat, truffle }, which will be a refined subset of tutorials as e2e tests.
  • removed e2e tests from workspace packages. This can avoid introducing unnecessary deps confusion, especially when hardhat is using ethers v6, while providers are using v5. We will run them as independent pkgs.
  • removed waffle examples since waffle is outdated. Also, waffle examples are essentially bodhi signer test, so instead we will put these tests in bodhi package directly

fix #737
fix #823
fix #916

Tests

In the scope of this PR, we only care about setting up the whole flow, so we only run a minimum test for each of the test section. We will actually transform and enable most of the old tests in the next PR.

The whole flow works as expected:

  • each test section runs in parallel and generates a coverage report
  • coverage reports are merged into one and is uploaded to codecov
  • test coverage is "penetrated" across packages. (i.e. when running e2e tests against eth rpc, test coverage also applies for eth-provider codes)

Todo

  • convert, fix, and enable all tests
  • deal with subql
  • some test coverage highlights seems to be off (though coverage data looks solid)

Copy link

codecov bot commented Sep 23, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@@ -58,7 +60,8 @@ export class BodhiSigner extends AbstractSigner implements TypedDataSigner {
}

static fromPair(provider: BodhiProvider, pair: KeyringPair): BodhiSigner {
return new BodhiSigner(provider, pair.address, new SubstrateSigner(provider.api.registry, pair));
const substrateAddr = encodeAddress(pair.address, provider.api.registry.chainSS58);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure why chopsticks and mandala behaves a little differently here: when doing extrinsic.signAsync, the payload address will use prefix: 42 on mandala, but prefix: 10 on chopsticks.

So I have to explicitly use prefix: 10 addr here, otherwise it won't be able to find the signer. Take a note here in case it breaks anything

Copy link
Collaborator Author

@shunjizhan shunjizhan Sep 25, 2024

Choose a reason for hiding this comment

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

ok mandala does have prefix:42, so this was indeed a bug before, just happened to work on mandala

@shunjizhan shunjizhan requested review from xlc and zjb0807 September 25, 2024 09:44
@shunjizhan shunjizhan marked this pull request as ready for review September 25, 2024 09:44
Copy link
Member

@zjb0807 zjb0807 left a comment

Choose a reason for hiding this comment

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

LGTM.

some tests for precompile contracts in hardhat-tutorials should be in the Acala repo.

@@ -4,13 +4,18 @@ on:
paths-ignore:
- '**/README.md'

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
Copy link
Member

Choose a reason for hiding this comment

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

why remove this?

Copy link
Collaborator Author

@shunjizhan shunjizhan Sep 25, 2024

Choose a reason for hiding this comment

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

for the concurrency, they run in parallel by default

sometimes cancel previous tests is slow (in old CI sometimes take a couple minutes), so I removed cancel-in-progress: true so new test can run immediately

Copy link
Collaborator Author

@shunjizhan shunjizhan Sep 25, 2024

Choose a reason for hiding this comment

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

we can probably close #832 too if we don't cancel previous run, ubuntu-latest is free anyways (sorry github)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if in the new setup, cancel previous run isn't that slow, we can also add this back, either way

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let's try add this back

mock-signature-host: true
runtime-log-level: 5
block: 7000000
wasm-override: ./chopsticks/wasm/acala-2250.wasm
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
wasm-override: ./chopsticks/wasm/acala-2250.wasm
wasm-override: ./chopsticks/wasm/acala-2260.wasm

Copy link
Collaborator Author

@shunjizhan shunjizhan Sep 25, 2024

Choose a reason for hiding this comment

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

old tests worked with runtime 2250, so let's control variable and upgrade to 2260 after all tests are converted and running (otherwise if some tests fail, we don't know if it's the test's issue, or runtime 2260's issue).

@shunjizhan
Copy link
Collaborator Author

shunjizhan commented Sep 25, 2024

some tests for precompile contracts in hardhat-tutorials should be in the Acala repo.

Yeah I agree, precompile functionalities should be tested in Acala's scope.

In bodhi's scope, it should be able to assume low level evm stuff works as expected. So bodhi tests only need to focus on toolings compatibilities (hardhat/truffle/viem/ethers/etc), as well as all different interactions with eth rpc

@shunjizhan shunjizhan merged commit 81996d4 into master Sep 26, 2024
13 checks passed
@shunjizhan shunjizhan deleted the tests-v2 branch September 26, 2024 02:59
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.

integrate viem example to CI integrate chopsticks e2e tests to CI test coverage
3 participants