Skip to content

Conversation

@mmsqe
Copy link
Contributor

@mmsqe mmsqe commented Jul 7, 2025

Description

Closes: #XXXX


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the main branch

Reviewers Checklist

All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code
  • reviewed content
  • tested instructions (if applicable)
  • confirmed all CI checks have passed

@mmsqe mmsqe marked this pull request as ready for review July 8, 2025 15:51
Copy link
Contributor

@zsystm zsystm Jul 10, 2025

Choose a reason for hiding this comment

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

Hey @mmsqe, thanks for your interest in improving the test structure — really appreciate the effort.

I have a question regarding the changes in your PR.

One of the main reasons I originally renamed the test files from *_test.go to test_*.go was to make these test suites reusable by other applications implementing the EvmApp interface (ref: #198). With this structure, client chains can write tests like the following, by simply providing their own CreateApp implementation:

func TestFeeMarketKeeperTestSuite(t *testing.T) {
	s := feemarket.NewTestKeeperTestSuite(CreateEvmd)
	suite.Run(t, s)
}

However, I noticed that in your PR, the x/feemarket tests were renamed back to *_test.go, which limits them to being runnable only within our own evmd application. Since Go does not expose *_test.go files as importable packages, other client chains won't be able to use them in their test suites.

Could you clarify your intent behind this change, or consider keeping the test_*.go structure so that the tests remain reusable across different implementations?

Thanks again for the contribution!

Copy link
Contributor Author

@mmsqe mmsqe Jul 11, 2025

Choose a reason for hiding this comment

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

The main issue is unable to detect those test packages, now we can
go test -v -run KeeperTestSuite/TestEndBlock ./tests/integration/x/feemarket
instead of
cd evmd && go test -tags=test -v -run TestFeeMarketKeeperTestSuite/TestEndBlock ./tests/integration

We could reuse feemarket test suite since the setup is simple, but it's better to separate SetupTest for each test suite for module like vm. Child test class could inherit SetupTest from its base class which is called by test runtime automatically. I can split the change by module to make review easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the benefit of being able to run tests directly via go test without needing to go through evmd, and agree that's a useful improvement for running tests within this repo.

That said, my original intention with the test_*.go structure was to make the test suites reusable outside of this repo — for example, by client chains like Mantra or Babylon that use cosmos/evm as a dependency. With the current *_test.go structure, those client chains won’t be able to import and use our test suites in their own repositories, because Go does not expose test files as importable packages.

To clarify: is the current structure you proposed still intended to support the following use case?

A client chain (e.g., MantraApp or BabylonApp) wants to write a minimal test like this in their repo:

func TestFeeMarketKeeperTestSuite(t *testing.T) {
	s := feemarket.NewTestKeeperTestSuite(CreateMantraApp)
	suite.Run(t, s)
}

...and have that test internally run the tests from /evmd/tests/integration/x/feemarket.

From what I can see in the current PR, this doesn't seem possible anymore — but please let me know if I’m missing something!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2efe7c1 evmd still could use testutil from evm instead of feemarket packge right?

Copy link
Contributor

@zsystm zsystm Jul 11, 2025

Choose a reason for hiding this comment

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

@mmsqe
I tried running the example you shared, but it results in:

=== RUN   TestFeeMarketKeeperTestSuite
    suite.go:234: warning: no tests to run
--- PASS: TestFeeMarketKeeperTestSuite (0.00s)
PASS

So it seems like there are no actual test cases being picked up.

To clarify — how would you make it possible for other applications (e.g. Mantra, Babylon) to run the tests defined in evm/tests/integration/x/feemarket/abci_test.go? (For other *_test.go also)

The goal is that client chains can run all the test cases defined in evm.

If the test file is named *_test.go, and lives in a separate test package, it likely won’t be included when imported externally — which defeats the purpose of making these test suites reusable.

Let me know if there's a workaround you had in mind.

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 just revert the test separation, since I didn't know other chain is using the exported test, but we do e2e with binary in python to make life easier.

@mmsqe mmsqe changed the title test: separate test suites for dedicated component test: cleanup EvmAppOptions related config Jul 11, 2025
Copy link
Contributor

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for cleaning config.
We need an one more approval from @vladjdk

@zsystm zsystm requested a review from vladjdk July 12, 2025 01:57
Copy link
Member

@vladjdk vladjdk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for cleaning this up :)

@vladjdk vladjdk merged commit 058eb6d into cosmos:main Jul 15, 2025
15 checks passed
zsystm pushed a commit to zsystm/evm that referenced this pull request Nov 2, 2025
* abci

* eip1559

* grpc

* msg

* params

* keeper

* integration

* rename

* align utils test that depends on NewTestSuite

bank

config

evm

* keep evmd -> evm dependency

* set antehandler before seal

* cleanup

* cleanup cfg

* cleanup

* cleanup

* reuse

* vm

* genesis

* fee

* param

* statedb

* add evmd

* revert

* test: cleanup EvmAppOptions related config
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