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

add: integration tests network suite #1728

Merged
merged 48 commits into from
Sep 18, 2023
Merged

add: integration tests network suite #1728

merged 48 commits into from
Sep 18, 2023

Conversation

facs95
Copy link
Contributor

@facs95 facs95 commented Aug 27, 2023

Description

This PR aims to propose a new global integration tests network and helpers to be used across the whole repository. The idea behind is this is to improve the quality of our current integration tests by first having a centralized code within our testing suites for:

  • Network setup
  • Query information from chain
  • Build, sign and broadcast transactions

Based on a bunch issues that we have experienced with misleading tests, this new design aims to force developers to simulate a more realistic scenario when building integration tests. The PR is trying to achieve this by:

  • Removing direct access to the network's context, keeper's and app forcing user to both, query the chain's information through GRPC and not being able to modify state without a transaction.
  • Forcing users to be explicit when changing the default configuration of the network.
  • Forcing users to be explicit when using the TxFactory (e.g being explicit when hard-coding gas or fees)

It will have to be expended as we start refractoring tests, what is included in the TxFactory and Grpc package specially is just an example of what I have identified will be needed for the first integration tests to be refractor.

This is just a first pass and a personal proposal, I expect this PR to be criticized and for it to be an active conversation so that when the PR gets merged we would have reached consensus on how we want integration tests to be built moving forward.

@facs95 facs95 requested a review from a team as a code owner August 27, 2023 22:51
@facs95 facs95 requested review from Vvaradinov and MalteHerrmann and removed request for a team August 27, 2023 22:51
testutil/integration/factory/factory.go Fixed Show fixed Hide fixed
testutil/integration/helpers.go Fixed Show fixed Hide fixed
// bondedAmt is the amount of tokens that each validator will have initially bonded
bondedAmt = sdktypes.TokensFromConsensusPower(1, types.PowerReduction)
// PrefundedAccountInitialBalance is the amount of tokens that each prefunded account has at genesis
PrefundedAccountInitialBalance = sdktypes.NewInt(int64(math.Pow10(18) * 4))

Check notice

Code scanning / CodeQL

Floating point arithmetic Note

Floating point arithmetic operations are not associative and a possible source of non-determinism
@facs95 facs95 changed the title add: integration tests network and helpers suite add: integration tests network suite Aug 27, 2023
Copy link
Contributor

@Vvaradinov Vvaradinov left a comment

Choose a reason for hiding this comment

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

Great work @facs95. Left some nits and typos

testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
testutil/integration/grpc/evm.go Outdated Show resolved Hide resolved
testutil/integration/grpc/revenue.go Outdated Show resolved Hide resolved
testutil/integration/network/setup.go Show resolved Hide resolved
Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

Great work @facs95!!

Left some nit comments

testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
}

var gasLimit uint64
if txArgs.Gas == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: should we add a flag to use the simulated gas?
Cause could be the case that we actually want to use a Gas = 0 in a test

Copy link
Contributor

Choose a reason for hiding this comment

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

I think carrying a separate flag would be more annoying than just put 1 everywhere we want to basically use no gas - or do you see a test case where it's necessary to specifically put 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree - I wanted to avoid passing flags all around

testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
testutil/integration/factory/helpers.go Show resolved Hide resolved
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM. I'd remove the Cosmos logic (DoCosmosTx, CosmosTxArgs) since we will deprecate that logic in favor of the EVM Extensions.

Please also add the License comment on each file

testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
facs95 and others added 5 commits August 29, 2023 18:39
Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>
Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>
Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>
Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>
@facs95
Copy link
Contributor Author

facs95 commented Aug 29, 2023

LGTM. I'd remove the Cosmos logic (DoCosmosTx, CosmosTxArgs) since we will deprecate that logic in favor of the EVM Extensions.

Please also add the License comment on each file

So we are not going to test modules through cosmos txs anymore? - Are we blocking all cosmos txs on the ante handler? @fedekunze. It might be sometime until we accomplish such migration, are you ok with maintaining this as we refractor the tests and then when that migration does happen we can remove the unnecessary helpers?

Will add the licenses 👍

Copy link
Contributor

@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

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

Awesome work @facs95! absolutely love to see this.

I notice that there are no tests for these test utilities yet, I think it definitely would make sense to check if they are all working as expected.

CHANGELOG.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
// GetBaseFee returns the base fee from the feemarket module.
func (gqh *IntegrationHandler) GetBaseFee() (*feemarkettypes.QueryBaseFeeResponse, error) {
feeMarketClient := gqh.network.GetFeeMarketClient()
return feeMarketClient.BaseFee(context.Background(), &feemarkettypes.QueryBaseFeeRequest{})
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we using an empty context here, and not gph.network.GetContext()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This query simulates a user that is making a query from an external agent that has no access to the context.. the network conext is requested internally from the GetFeeMarketClient to simulate that grpc client

testutil/integration/grpc/grpc.go Show resolved Hide resolved
testutil/integration/grpc/grpc.go Outdated Show resolved Hide resolved
testutil/integration/grpc/grpc.go Outdated Show resolved Hide resolved
testutil/integration/grpc/account.go Outdated Show resolved Hide resolved
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
@@ -93,6 +96,7 @@ func createEvmosApp(chainID string) *app.Evmos {
)
}

// createStakingValidator creates a staking validator from the given tm validator and bonded
func createStakingValidator(val *tmtypes.Validator, amtOfBondedTokens sdkmath.Int) (stakingtypes.Validator, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func createStakingValidator(val *tmtypes.Validator, amtOfBondedTokens sdkmath.Int) (stakingtypes.Validator, error) {
func createStakingValidator(val *tmtypes.Validator, bondedAmt sdkmath.Int) (stakingtypes.Validator, error) {

I would suggest to update this input name to be consistent with the function createStakingValidators.

Copy link
Contributor

@0xstepit 0xstepit left a comment

Choose a reason for hiding this comment

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

Great job @facs95 !

facs95 and others added 3 commits September 12, 2023 11:03
Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>
Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

LGTM! Great work @facs95!! 🙌

Copy link
Contributor

@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

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

Just some more small nits - this is great, super excited to get it merged! 🚀

testutil/integration/factory/factory.go Outdated Show resolved Hide resolved
testutil/integration/keyring/keyring.go Outdated Show resolved Hide resolved
testutil/integration/keyring/keyring.go Outdated Show resolved Hide resolved
testutil/integration/keyring/keyring.go Outdated Show resolved Hide resolved
testutil/integration/keyring/keyring.go Outdated Show resolved Hide resolved
facs95 and others added 8 commits September 13, 2023 10:15
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
@facs95 facs95 merged commit 66eaddb into main Sep 18, 2023
33 of 34 checks passed
@facs95 facs95 deleted the facs95/integration-test branch September 18, 2023 12:39
@Vvaradinov
Copy link
Contributor

https://github.com/Mergifyio backport release/v15.0.x

@mergify
Copy link
Contributor

mergify bot commented Oct 16, 2023

backport release/v15.0.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Oct 16, 2023
* integration interface

* grpc and factory packages

* fix linter issues

* Update testutil/integration/factory/factory.go

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>

* Update testutil/integration/grpc/evm.go

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>

* Update testutil/integration/grpc/revenue.go

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>

* remove unnecesary comment

* refractor based on comments

* fix lint

* Update testutil/integration/factory/factory.go

Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>

* fix comment

* add licenses

* add changelog

* Update CHANGELOG.md

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/helpers.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/grpc/grpc.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* review comments

* add error wrappers

* refractor executeCosmosTx function

* keyring package

* Update testutil/integration/network/setup.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* rename NextBlock

* fix typos

* unified slice allocation thorugh setup

* add doc comments

* add chainID checker

* Update testutil/integration/network/abci.go

Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>

* fix float arithmetic

* fix licenses

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/keyring/keyring.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/keyring/keyring.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/keyring/keyring.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/keyring/keyring.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* rename var createStakingValidator

---------

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>
Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com>
(cherry picked from commit 66eaddb)

# Conflicts:
#	CHANGELOG.md
Vvaradinov added a commit that referenced this pull request Oct 16, 2023
* add: integration tests network suite (#1728)

* integration interface

* grpc and factory packages

* fix linter issues

* Update testutil/integration/factory/factory.go

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>

* Update testutil/integration/grpc/evm.go

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>

* Update testutil/integration/grpc/revenue.go

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>

* remove unnecesary comment

* refractor based on comments

* fix lint

* Update testutil/integration/factory/factory.go

Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>

* fix comment

* add licenses

* add changelog

* Update CHANGELOG.md

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/helpers.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/grpc/grpc.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* review comments

* add error wrappers

* refractor executeCosmosTx function

* keyring package

* Update testutil/integration/network/setup.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* rename NextBlock

* fix typos

* unified slice allocation thorugh setup

* add doc comments

* add chainID checker

* Update testutil/integration/network/abci.go

Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>

* fix float arithmetic

* fix licenses

* Update testutil/integration/factory/factory.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/keyring/keyring.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/keyring/keyring.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/keyring/keyring.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update testutil/integration/keyring/keyring.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* rename var createStakingValidator

---------

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>
Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com>
(cherry picked from commit 66eaddb)

# Conflicts:
#	CHANGELOG.md

* CHANGELOG

* fix CHANGELOG

---------

Co-authored-by: Freddy Caceres <facs95@gmail.com>
Co-authored-by: Vlad <vladislav.varadinov@gmail.com>
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.

None yet

6 participants