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

chore (tests): move utilities from tests folder to testutil package #1393

Merged
merged 9 commits into from
Feb 15, 2023

Conversation

MalteHerrmann
Copy link
Contributor

@MalteHerrmann MalteHerrmann commented Feb 14, 2023

Description

Going along with the current restructuring of the way we are handling tests, this PR moves the utility functions in tests/signer.go to testutil/signer.go.

This way, the tests folder only contains concrete meta-testing suites like the ledger integration tests on the Evmos side as well as the E2E tests and all utility functions are contained in the testutil folder.

Other than that, this PR

  • fixes the use of the context's chain id in the app/ante/... test files, that was accidentally removed when fixing merge conflicts on a prior PR.
  • fixes import cycles after moving the utilities from the tests folder, because the testutil package imports several types packages, which then use the testutils for testing

@MalteHerrmann
Copy link
Contributor Author

currently running into import cycles with several tests after this refactor, but I still think it's much cleaner this way - currently fixing those

@facs95
Copy link
Contributor

facs95 commented Feb 15, 2023

I know this PR is not ready but as I see the progress and I am trying to reorganize this as well - I wanted to open the discussion about the directories you are trying to use.

I agree with the architecture proposed in this ticket https://linear.app/evmos/issue/ENG-1502/evmos-testing-setup in which the utils package for the new common testing refractor is under test/util .

I think that would make more sense than maintaining a ./testutil package and directory. Would be cleaner imo to have everything under test/ but well organized, similar to the description in the ticket

@MalteHerrmann
Copy link
Contributor Author

@facs95 I would actually more be a proponent of keeping the utilities and the actual tests separate just to keep everything clean. The ./tests folder contains the Evmos side of the Ledger integration tests as well as upgrade tests. Having a clear directory for reusable code like ./testutil would IMO also be a good thing for the Evmos SDK just so people don't get confused when navigating the folders. But def. open to discussions about this!

The work to be done here in terms of having the import cycles solved must be done in either of those two options, so I'll continue with this for now ✌️

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #1393 (d4da6d1) into main (c667044) will not change coverage.
The diff coverage is 85.71%.

❗ Current head d4da6d1 differs from pull request most recent head 4f1f521. Consider uploading reports for the commit 4f1f521 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1393   +/-   ##
=======================================
  Coverage   72.42%   72.42%           
=======================================
  Files         260      260           
  Lines       17685    17685           
=======================================
  Hits        12808    12808           
  Misses       4308     4308           
  Partials      569      569           
Impacted Files Coverage Δ
x/erc20/types/params.go 85.71% <66.66%> (ø)
x/evm/types/dynamic_fee_tx.go 90.37% <100.00%> (ø)
x/evm/types/legacy_tx.go 91.72% <100.00%> (ø)
x/evm/types/tx_data.go 97.91% <100.00%> (ø)

@MalteHerrmann MalteHerrmann marked this pull request as ready for review February 15, 2023 09:28
@MalteHerrmann MalteHerrmann requested review from GAtom22, fedekunze and Vvaradinov and removed request for a team February 15, 2023 09:28
Copy link
Contributor

@facs95 facs95 left a comment

Choose a reason for hiding this comment

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

Makes sense - I am going to give this an approval not to block other refractos as this separation from the integration tests to the utilities had to be done anyways regardless of the naming. Awesome job solving the cycles!

@facs95
Copy link
Contributor

facs95 commented Feb 15, 2023

Can you please add a changelog?

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.

ACK pending changelog entry under Improvements

@MalteHerrmann MalteHerrmann enabled auto-merge (squash) February 15, 2023 13:43
@fedekunze fedekunze merged commit 321af02 into main Feb 15, 2023
@fedekunze fedekunze deleted the malte/move-utils-in-tests-folder-into-testutils branch February 15, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants