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

test: migrate remaining e2e tests to integration tests #22364

Merged
merged 8 commits into from
Oct 27, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 0 additions & 31 deletions .github/workflows/test.yml
Copy link
Member

@julienrbrt julienrbrt Oct 25, 2024

Choose a reason for hiding this comment

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

Can you remove the test-e2e from the needs?

Original file line number Diff line number Diff line change
Expand Up @@ -113,37 +113,6 @@ jobs:
name: "${{ github.sha }}-integration-coverage"
path: ./tests/integration-profile.out

test-e2e:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version: "1.23"
check-latest: true
cache: true
cache-dependency-path: go.sum
- uses: technote-space/[email protected]
id: git_diff
with:
PATTERNS: |
**/*.go
go.mod
go.sum
**/go.mod
**/go.sum
**/Makefile
Makefile
- name: e2e tests
if: env.GIT_DIFF
run: |
make test-e2e-cov
- uses: actions/upload-artifact@v3
if: env.GIT_DIFF
with:
name: "${{ github.sha }}-e2e-coverage"
path: ./tests/e2e-profile.out

test-system: # v2 system tests are in v2-test.yml
runs-on: ubuntu-latest
steps:
Expand Down
6 changes: 0 additions & 6 deletions tests/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,3 @@ test-integration:

test-integration-cov:
go test ./integration/... -timeout 30m -coverpkg=../... -coverprofile=integration-profile.out -covermode=atomic

test-e2e:
go test ./e2e/... -mod=readonly -timeout 30m -race -tags='e2e'

test-e2e-cov:
go test ./e2e/... -mod=readonly -timeout 30m -race -tags='e2e' -coverpkg=../... -coverprofile=e2e-profile.out -covermode=atomic
11 changes: 0 additions & 11 deletions tests/e2e/accounts/lockup/lockup_account_test.go

This file was deleted.

Copy link
Member

Choose a reason for hiding this comment

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

looks like those was pulled in with build failures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests required utils functions which were deleted earlier with this commit, which got unnoticed. Will raise a PR with fix.

File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

func (s *E2ETestSuite) TestContinuousLockingAccount() {
func (s *IntegrationTestSuite) TestContinuousLockingAccount() {
t := s.T()
app := setupApp(t)
currentTime := time.Now()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

func (s *E2ETestSuite) TestDelayedLockingAccount() {
func (s *IntegrationTestSuite) TestDelayedLockingAccount() {
t := s.T()
app := setupApp(t)
currentTime := time.Now()
Expand Down
11 changes: 11 additions & 0 deletions tests/integration/accounts/lockup/lockup_account_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package lockup

import (
"testing"

"github.com/stretchr/testify/suite"
)

func TestIntegrationTestSuite(t *testing.T) {
suite.Run(t, NewIntegrationTestSuite())
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

func (s *E2ETestSuite) TestPeriodicLockingAccount() {
func (s *IntegrationTestSuite) TestPeriodicLockingAccount() {
t := s.T()
app := setupApp(t)
currentTime := time.Now()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

func (s *E2ETestSuite) TestPermanentLockingAccount() {
func (s *IntegrationTestSuite) TestPermanentLockingAccount() {
t := s.T()
app := setupApp(t)
currentTime := time.Now()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,23 @@ var (
accOwner = sdk.AccAddress(ownerAddr)
)

type E2ETestSuite struct {
type IntegrationTestSuite struct {
suite.Suite

app *simapp.SimApp
}

func NewE2ETestSuite() *E2ETestSuite {
return &E2ETestSuite{}
func NewIntegrationTestSuite() *IntegrationTestSuite {
return &IntegrationTestSuite{}
}

func (s *E2ETestSuite) SetupSuite() {
s.T().Log("setting up e2e test suite")
func (s *IntegrationTestSuite) SetupSuite() {
s.T().Log("setting up integration test suite")
s.app = setupApp(s.T())
}

func (s *E2ETestSuite) TearDownSuite() {
s.T().Log("tearing down e2e test suite")
func (s *IntegrationTestSuite) TearDownSuite() {
s.T().Log("tearing down integration test suite")
Comment on lines +33 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing setup and teardown methods

While the basic structure is in place, consider these improvements:

  1. Add validation checks in SetupSuite to ensure the app is properly initialized
  2. Implement proper cleanup in TearDownSuite to prevent potential test interference
  3. Add logging of important test configuration parameters

Example enhancement:

 func (s *IntegrationTestSuite) SetupSuite() {
     s.T().Log("setting up integration test suite")
     s.app = setupApp(s.T())
+    // Validate app initialization
+    require.NotNil(s.T(), s.app.AccountsKeeper)
+    require.NotNil(s.T(), s.app.BankKeeper)
+    s.T().Log("app initialized with account keeper and bank keeper")
 }

 func (s *IntegrationTestSuite) TearDownSuite() {
     s.T().Log("tearing down integration test suite")
+    // Clean up any remaining state
+    s.app = nil
 }

Committable suggestion was skipped due to low confidence.

}

func setupApp(t *testing.T) *simapp.SimApp {
Expand All @@ -45,21 +45,21 @@ func setupApp(t *testing.T) *simapp.SimApp {
return app
}

func (s *E2ETestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error {
func (s *IntegrationTestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error {
_, err := app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil)
return err
}
Comment on lines +48 to 51
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling in executeTx

Consider adding context to the error return to help with debugging test failures.

 func (s *IntegrationTestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error {
 	_, err := app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil)
-	return err
+	if err != nil {
+		return fmt.Errorf("failed to execute transaction: %w", err)
+	}
+	return nil
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (s *IntegrationTestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error {
_, err := app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil)
return err
}
func (s *IntegrationTestSuite) executeTx(ctx sdk.Context, msg sdk.Msg, app *simapp.SimApp, accAddr, sender []byte) error {
_, err := app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil)
if err != nil {
return fmt.Errorf("failed to execute transaction: %w", err)
}
return nil
}


func (s *E2ETestSuite) queryAcc(ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (transaction.Msg, error) {
func (s *IntegrationTestSuite) queryAcc(ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (transaction.Msg, error) {
resp, err := app.AccountsKeeper.Query(ctx, accAddr, req)
return resp, err
}
Comment on lines +53 to 56
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add type safety to queryAcc method

The method could benefit from generic type parameters to ensure type safety at compile time.

-func (s *IntegrationTestSuite) queryAcc(ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (transaction.Msg, error) {
+func (s *IntegrationTestSuite) queryAcc[T transaction.Msg](ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (T, error) {
 	resp, err := app.AccountsKeeper.Query(ctx, accAddr, req)
-	return resp, err
+	if err != nil {
+		var zero T
+		return zero, fmt.Errorf("query failed: %w", err)
+	}
+	result, ok := resp.(T)
+	if !ok {
+		var zero T
+		return zero, fmt.Errorf("unexpected response type: got %T, want %T", resp, zero)
+	}
+	return result, nil
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (s *IntegrationTestSuite) queryAcc(ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (transaction.Msg, error) {
resp, err := app.AccountsKeeper.Query(ctx, accAddr, req)
return resp, err
}
func (s *IntegrationTestSuite) queryAcc[T transaction.Msg](ctx sdk.Context, req sdk.Msg, app *simapp.SimApp, accAddr []byte) (T, error) {
resp, err := app.AccountsKeeper.Query(ctx, accAddr, req)
if err != nil {
var zero T
return zero, fmt.Errorf("query failed: %w", err)
}
result, ok := resp.(T)
if !ok {
var zero T
return zero, fmt.Errorf("unexpected response type: got %T, want %T", resp, zero)
}
return result, nil
}


func (s *E2ETestSuite) fundAccount(app *simapp.SimApp, ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) {
func (s *IntegrationTestSuite) fundAccount(app *simapp.SimApp, ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) {
require.NoError(s.T(), testutil.FundAccount(ctx, app.BankKeeper, addr, amt))
}

func (s *E2ETestSuite) queryLockupAccInfo(ctx sdk.Context, app *simapp.SimApp, accAddr []byte) *types.QueryLockupAccountInfoResponse {
func (s *IntegrationTestSuite) queryLockupAccInfo(ctx sdk.Context, app *simapp.SimApp, accAddr []byte) *types.QueryLockupAccountInfoResponse {
req := &types.QueryLockupAccountInfoRequest{}
resp, err := s.queryAcc(ctx, req, app, accAddr)
require.NoError(s.T(), err)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

func TestE2ETestSuite(t *testing.T) {
suite.Run(t, NewE2ETestSuite())
func TestIntegrationTestSuite(t *testing.T) {
suite.Run(t, NewIntegrationTestSuite())
}

// TestSimpleSendProposal creates a multisig account with 1 member, sends a tx, votes and executes it.
func (s *E2ETestSuite) TestSimpleSendProposal() {
func (s *IntegrationTestSuite) TestSimpleSendProposal() {
ctx := sdk.NewContext(s.app.CommitMultiStore(), false, s.app.Logger()).WithHeaderInfo(header.Info{
Time: time.Now(),
})
Expand Down Expand Up @@ -110,7 +110,7 @@ func (s *E2ETestSuite) TestSimpleSendProposal() {

// TestConfigUpdate creates a multisig with 1 member, adds 2 more members and
// changes the config to require 2/3 majority (also through a proposal).
func (s *E2ETestSuite) TestConfigUpdate() {
func (s *IntegrationTestSuite) TestConfigUpdate() {
ctx := sdk.NewContext(s.app.CommitMultiStore(), false, s.app.Logger()).WithHeaderInfo(header.Info{
Time: time.Now(),
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,19 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

type E2ETestSuite struct {
type IntegrationTestSuite struct {
suite.Suite

app *simapp.SimApp
members []sdk.AccAddress
membersAddr []string
}

func NewE2ETestSuite() *E2ETestSuite {
return &E2ETestSuite{}
func NewIntegrationTestSuite() *IntegrationTestSuite {
return &IntegrationTestSuite{}
}
Comment on lines +29 to 31
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider initializing fields in the constructor.

The constructor could be enhanced to initialize the members slice and potentially the app instance, making the test suite setup more explicit.

 func NewIntegrationTestSuite() *IntegrationTestSuite {
-    return &IntegrationTestSuite{}
+    return &IntegrationTestSuite{
+        members:     make([]sdk.AccAddress, 0),
+        membersAddr: make([]string, 0),
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func NewIntegrationTestSuite() *IntegrationTestSuite {
return &IntegrationTestSuite{}
}
func NewIntegrationTestSuite() *IntegrationTestSuite {
return &IntegrationTestSuite{
members: make([]sdk.AccAddress, 0),
membersAddr: make([]string, 0),
}
}


func (s *E2ETestSuite) SetupSuite() {
func (s *IntegrationTestSuite) SetupSuite() {
s.app = setupApp(s.T())

s.members = []sdk.AccAddress{}
Expand All @@ -43,31 +43,31 @@ func (s *E2ETestSuite) SetupSuite() {
}
}

func (s *E2ETestSuite) TearDownSuite() {}
func (s *IntegrationTestSuite) TearDownSuite() {}

func setupApp(t *testing.T) *simapp.SimApp {
t.Helper()
app := simapp.Setup(t, false)
return app
}

func (s *E2ETestSuite) executeTx(ctx context.Context, msg sdk.Msg, accAddr, sender []byte) error {
func (s *IntegrationTestSuite) executeTx(ctx context.Context, msg sdk.Msg, accAddr, sender []byte) error {
_, err := s.app.AccountsKeeper.Execute(ctx, accAddr, sender, msg, nil)
return err
}

func (s *E2ETestSuite) queryAcc(ctx context.Context, req sdk.Msg, accAddr []byte) (transaction.Msg, error) {
func (s *IntegrationTestSuite) queryAcc(ctx context.Context, req sdk.Msg, accAddr []byte) (transaction.Msg, error) {
resp, err := s.app.AccountsKeeper.Query(ctx, accAddr, req)
return resp, err
}

func (s *E2ETestSuite) fundAccount(ctx context.Context, addr sdk.AccAddress, amt sdk.Coins) {
func (s *IntegrationTestSuite) fundAccount(ctx context.Context, addr sdk.AccAddress, amt sdk.Coins) {
require.NoError(s.T(), testutil.FundAccount(ctx, s.app.BankKeeper, addr, amt))
}

// initAccount initializes a multisig account with the given members and powers
// and returns the account address
func (s *E2ETestSuite) initAccount(ctx context.Context, sender []byte, membersPowers map[string]uint64) ([]byte, string) {
func (s *IntegrationTestSuite) initAccount(ctx context.Context, sender []byte, membersPowers map[string]uint64) ([]byte, string) {
s.fundAccount(ctx, sender, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000000))})

members := []*v1.Member{}
Expand Down Expand Up @@ -95,7 +95,7 @@ func (s *E2ETestSuite) initAccount(ctx context.Context, sender []byte, membersPo
}

// createProposal
func (s *E2ETestSuite) createProposal(ctx context.Context, accAddr, sender []byte, msgs ...*codectypes.Any) {
func (s *IntegrationTestSuite) createProposal(ctx context.Context, accAddr, sender []byte, msgs ...*codectypes.Any) {
propReq := &v1.MsgCreateProposal{
Proposal: &v1.Proposal{
Title: "test",
Expand All @@ -107,7 +107,7 @@ func (s *E2ETestSuite) createProposal(ctx context.Context, accAddr, sender []byt
s.NoError(err)
}

func (s *E2ETestSuite) executeProposal(ctx context.Context, accAddr, sender []byte, proposalID uint64) error {
func (s *IntegrationTestSuite) executeProposal(ctx context.Context, accAddr, sender []byte, proposalID uint64) error {
execReq := &v1.MsgExecuteProposal{
ProposalId: proposalID,
}
Expand Down
Copy link
Member

Choose a reason for hiding this comment

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

and this one

File renamed without changes.
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
package keeper
package keeper_test

import (
"testing"

"github.com/stretchr/testify/require"

authTest "github.com/cosmos/cosmos-sdk/tests/integration/auth/keeper"
"github.com/cosmos/cosmos-sdk/testutil/network"
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any reason for having AppConfig in a different package?? looks like some have it in the same package while others don't 🤷‍♂️

Copy link
Contributor Author

@akhilkumarpilli akhilkumarpilli Oct 25, 2024

Choose a reason for hiding this comment

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

It's in the same package. Tests are with keeper_test package name and app_config is with keeper package name.

"github.com/cosmos/cosmos-sdk/x/auth/types"
)

func TestAccountRetriever(t *testing.T) {
cfg, err := network.DefaultConfigWithAppConfig(AppConfig)
cfg, err := network.DefaultConfigWithAppConfig(authTest.AppConfig)
require.NoError(t, err)
cfg.NumValidators = 1

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package keeper
package keeper_test

import (
"testing"
Expand All @@ -8,6 +8,7 @@ import (
"cosmossdk.io/depinject"
"cosmossdk.io/log"

authTest "github.com/cosmos/cosmos-sdk/tests/integration/auth/keeper"
simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/keeper"
Expand All @@ -19,7 +20,7 @@ func BenchmarkAccountMapperGetAccountFound(b *testing.B) {
app, err := simtestutil.Setup(
depinject.Configs(
depinject.Supply(log.NewNopLogger()),
AppConfig,
authTest.AppConfig,
),
&accountKeeper,
)
Expand Down Expand Up @@ -48,7 +49,7 @@ func BenchmarkAccountMapperSetAccount(b *testing.B) {
app, err := simtestutil.Setup(
depinject.Configs(
depinject.Supply(log.NewNopLogger()),
AppConfig,
authTest.AppConfig,
), &accountKeeper)
require.NoError(b, err)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package keeper
package keeper_test

import (
"testing"
Expand All @@ -8,6 +8,7 @@ import (
"cosmossdk.io/depinject"
"cosmossdk.io/log"

authTest "github.com/cosmos/cosmos-sdk/tests/integration/auth/keeper"
simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
"github.com/cosmos/cosmos-sdk/x/auth/keeper"
"github.com/cosmos/cosmos-sdk/x/auth/types"
Expand All @@ -17,7 +18,7 @@ func TestItCreatesModuleAccountOnInitBlock(t *testing.T) {
var accountKeeper keeper.AccountKeeper
app, err := simtestutil.SetupAtGenesis(
depinject.Configs(
AppConfig,
authTest.AppConfig,
depinject.Supply(log.NewNopLogger()),
),
&accountKeeper)
Expand Down
Loading
Loading