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

feat: integration test helpers #15556

Merged
merged 23 commits into from
Mar 31, 2023
Merged

feat: integration test helpers #15556

merged 23 commits into from
Mar 31, 2023

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Mar 27, 2023

Description

Closes: #14676

I have not pushed everything yet: (#15556 (comment))

  • refactor tests/integration (here I am doing staking and evidence)
  • keep test coverage the same (I am currently moving tests around to keep the area tested)

I am as well exploring a way to register the msg server and query server directly when creating a integration app.


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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@julienrbrt julienrbrt changed the title feat: integration test framework feat: integration test framework (tentative implementation) Mar 27, 2023
@julienrbrt
Copy link
Member Author

julienrbrt commented Mar 28, 2023

Opening already, so possibly we can migrate module in follow-ups.
I'm still continuing the work I mentioned in the issue; however, I'll open PRs against this branch.

@julienrbrt julienrbrt marked this pull request as ready for review March 28, 2023 14:06
@julienrbrt julienrbrt requested a review from a team as a code owner March 28, 2023 14:06
@julienrbrt julienrbrt requested review from likhita-809, mark-rushakoff and tac0turtle and removed request for aaronc March 28, 2023 14:06
@julienrbrt julienrbrt changed the title feat: integration test framework (tentative implementation) feat: integration test helpers (tentative implementation) Mar 28, 2023
@kocubinski kocubinski self-assigned this Mar 28, 2023
:::note
You can as well use the `AppConfig` `configurator` for creating an `AppConfig` [inline](https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/x/slashing/app_test.go#L54-L62). There no difference between those two ways, use whichever you prefer.
:::
Integration tests interact with the tested module via the defined `Msg` and `Query` services. The result of the test can be verified by checking the state of the application, by checking the emitted events or the response. It is adviced to combine two of these methods to verify the result of the test.
Copy link
Member

Choose a reason for hiding this comment

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

will we document how to set things up here or elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here, as they are for testing modules integrations.

Copy link
Member

@mark-rushakoff mark-rushakoff left a comment

Choose a reason for hiding this comment

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

I wasn't able to read all the way through this before my end of day, but I did leave two comments. I can take another look tomorrow.

tests/integration/slashing/integration.go Outdated Show resolved Hide resolved
testutil/integration/router.go Outdated Show resolved Hide resolved
Copy link
Member

@mark-rushakoff mark-rushakoff left a comment

Choose a reason for hiding this comment

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

I think I follow what's going on here now. Structure and organization looks fine overall.

Let's change the test labeled example to an example test so it shows up at pkg.go.dev, and there were a few other small details I called out to help consumers follow intent here.

testutil/integration/example_test.go Outdated Show resolved Hide resolved
testutil/integration/example_test.go Outdated Show resolved Hide resolved
testutil/integration/example_test.go Outdated Show resolved Hide resolved
testutil/integration/router.go Outdated Show resolved Hide resolved
testutil/integration/router.go Outdated Show resolved Hide resolved
testutil/integration/router.go Show resolved Hide resolved
testutil/integration/router.go Outdated Show resolved Hide resolved
testutil/integration/router.go Show resolved Hide resolved
testutil/integration/example_test.go Outdated Show resolved Hide resolved
testutil/integration/router.go Outdated Show resolved Hide resolved
@julienrbrt
Copy link
Member Author

Thank you for the great feedback @mark-rushakoff! I've added your suggestions.

@julienrbrt julienrbrt changed the title feat: integration test helpers (tentative implementation) feat: integration test helpers Mar 31, 2023
@julienrbrt julienrbrt enabled auto-merge (squash) March 31, 2023 15:40
Copy link
Member

@mark-rushakoff mark-rushakoff left a comment

Choose a reason for hiding this comment

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

I didn't read the markdown changes closely, nor did I look closely at the changes in tests/integration/slashing/keeper/keeper_test.go. But the additions to testutil/integration look fine to me.

I noted a couple other small typos here, but otherwise I think this can be merged.

testutil/integration/example_test.go Outdated Show resolved Hide resolved
testutil/integration/example_test.go Outdated Show resolved Hide resolved
testutil/integration/router.go Outdated Show resolved Hide resolved
testutil/integration/router.go Outdated Show resolved Hide resolved
@julienrbrt julienrbrt enabled auto-merge (squash) March 31, 2023 15:44
Copy link
Contributor

@likhita-809 likhita-809 left a comment

Choose a reason for hiding this comment

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

nice work julien

docs/docs/building-modules/16-testing.md Outdated Show resolved Hide resolved
testutil/integration/doc.go Outdated Show resolved Hide resolved
testutil/integration/example_test.go Outdated Show resolved Hide resolved
testutil/integration/router.go Outdated Show resolved Hide resolved
@julienrbrt julienrbrt enabled auto-merge (squash) March 31, 2023 17:17
@julienrbrt julienrbrt merged commit aeaa301 into main Mar 31, 2023
@julienrbrt julienrbrt deleted the julien/integration branch March 31, 2023 18:02
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Good to see this work happening. Would be nice to align it with the direction core API is going. I think we also need ways to advance blocks so that begin and end blockers get called

testutil/integration/router.go Show resolved Hide resolved
return app.ctx
}

func (app *App) QueryHelper() *baseapp.QueryServiceTestHelper {
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just return grpc.ClientConnInterface?

@julienrbrt
Copy link
Member Author

Good to see this work happening. Would be nice to align it with the direction core API is going. I think we also need ways to advance blocks so that begin and end blockers get called

Right, Marko mentioned that too. I'll open a PR about the last comment.

About Core API I don't think we can update it here just yet.

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.

Research an integration framework and create PoC
6 participants