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

Fix internal tests. #1969

Merged
merged 9 commits into from
May 13, 2024
Merged

Fix internal tests. #1969

merged 9 commits into from
May 13, 2024

Conversation

SpicyLemon
Copy link
Contributor

@SpicyLemon SpicyLemon commented May 10, 2024

Description

This PR fixes the rest of the internal tests and makes them required to pass again.

Related to:


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

Summary by CodeRabbit

  • Refactor
    • Enhanced handling of fee grant and payer functionality for improved transaction processing.
  • Tests
    • Updated test structures and methodologies to align with the latest coding standards and practices.
  • Chores
    • Removed outdated comments and refined codebase for better maintainability.

@SpicyLemon SpicyLemon marked this pull request as ready for review May 10, 2024 23:48
@SpicyLemon SpicyLemon requested a review from a team as a code owner May 10, 2024 23:48
Copy link
Contributor

coderabbitai bot commented May 10, 2024

Warning

Rate Limit Exceeded

@SpicyLemon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 49 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between ee0b7cc and e6ef670.

Walkthrough

The recent updates focus on refining the testing and functionality of the Provenance blockchain modules. Key changes include the adjustment of function signatures to enhance flexibility, the addition of new methods to structures for more detailed data handling, and the streamlining of imports and code structure for clarity and efficiency. The removal of unnecessary exclusions and comments also indicates a move towards cleaner, more maintainable code.

Changes

File Path Change Summary
.github/workflows/test.yml Removed exclusion for 'github.com/provenance-io/provenance/internal/antewrapper'.
internal/antewrapper/... Added imports, new struct fields, and methods. Modified function signatures for enhanced codec handling.
internal/handlers/... Updated import aliases, modified function signatures to use updated imports, and enhanced transaction creation functions.
internal/handlers/msg_service_router.go Removed a TODO comment regarding service registration.
internal/antewrapper/testutil_test.go, internal/antewrapper/prov_feegrant_test.go Updated import aliases and function handling, improved test structuring and error handling.

🐇✨
In the realm of code, where logic is king,
A rabbit hopped by, making changes with a swing.
Imports aligned, and structures refined,
With every commit, the code did shine.
Hop, skip, a push, and a merge,
Celebrate the craft, as improvements surge! 🌟🎉
🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@SpicyLemon SpicyLemon enabled auto-merge (squash) May 10, 2024 23:53
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Out of diff range and nitpick comments (3)
internal/antewrapper/testutil_test.go (1)

Line range hint 121-158: Refactor the CreateTestTx method to improve readability and error handling.

func (s *AnteTestSuite) CreateTestTx(privs []cryptotypes.PrivKey, accNums []uint64, accSeqs []uint64, chainID string) (authsigning.Tx, error) {
	var sigsV2 []signing.SignatureV2
	for i, priv := range privs {
		sigV2 := signing.SignatureV2{
			PubKey: priv.PubKey(),
			Data: &signing.SingleSignatureData{
				SignMode:  signing.SignMode_SIGN_MODE_DIRECT,
				Signature: nil,
			},
			Sequence: accSeqs[i],
		}
		sigsV2 = append(sigsV2, sigV2)
	}
	if err := s.txBuilder.SetSignatures(sigsV2...); err != nil {
		return nil, err
	}

	sigsV2 = nil
	for i, priv := range privs {
		signerData := authsigning.SignerData{
			ChainID:       chainID,
			AccountNumber: accNums[i],
			Sequence:      accSeqs[i],
		}
		sigV2, err := tx.SignWithPrivKey(s.ctx, signing.SignMode_SIGN_MODE_DIRECT, signerData, s.txBuilder, priv, s.clientCtx.TxConfig, accSeqs[i])
		if err != nil {
			return nil, err
		}
		sigsV2 = append(sigsV2, sigV2)
	}
	if err := s.txBuilder.SetSignatures(sigsV2...); err != nil {
		return nil, err
	}

	return s.txBuilder.GetTx(), nil
}

This refactoring simplifies the method by removing redundant code and improving the structure for better readability and maintainability.

internal/antewrapper/prov_feegrant_test.go (1)

Line range hint 94-156: Enhance the clarity of test cases by improving naming and structure.

cases := []struct {
	name       string
	signerKey  cryptotypes.PrivKey
	signer     sdk.AccAddress
	feeAccount sdk.AccAddress
	fee        int64
	expInErr   []string
}{
	{
		name:      "insufficient funds without grants",
		signerKey: priv1,
		signer:    addr1,
		fee:       defaultGas,
		expInErr:  []string{"10stake", defaultGasStr, "insufficient funds"},
	},
	{
		name:      "sufficient funds",
		signerKey: priv2,
		signer:    addr2,
		fee:       defaultGas,
		expInErr:  nil,
	},
	{
		name:      "no account with fee",
		signerKey: priv3,
		signer:    addr3,
		fee:       defaultGas,
		expInErr:  []string{"0stake", defaultGasStr, "insufficient funds"},
	},
	{
		name:      "no fee and no account",
		signerKey: priv5,
		signer:    addr5,
		fee:       0,
		expInErr:  []string{"fee payer address", addr5.String(), "does not exist"},
	},
	{
		name:       "valid fee grant without account",
		signerKey:  priv3,
		signer:     addr3,
		feeAccount: addr2,
		fee:        defaultGas,
		expInErr:   nil,
	},
	{
		name:       "missing fee grant",
		signerKey:  priv3,
		signer:     addr3,
		feeAccount: addr1,
		fee:        defaultGas,
		expInErr: []string{
			"failed to use fee grant",
			fmt.Sprintf("granter: %s", addr1),
			fmt.Sprintf("grantee: %s", addr3),
			fmt.Sprintf(`fee: "%s"`, defaultGasStr),
			`msgs: ["/testpb.TestMsg"]`,
			"fee-grant not found",
		},
	},
	{
		name:       "allowance less than requested fee",
		signerKey:  priv4,
		signer:     addr4,
		feeAccount: addr2,
		fee:        defaultGas,
		expInErr: []string{
			"failed to use fee grant",
			fmt.Sprintf("granter: %s", addr2),
			fmt.Sprintf("grantee: %s", addr4),
			fmt.Sprintf(`fee: "%s"`, defaultGasStr),
			`msgs: ["/testpb.TestMsg"]`,
			"fee limit exceeded",
		},
	},
	{
		name:       "granter cannot cover fee grant",
		signerKey:  priv4,
		signer:     addr4,
		feeAccount: addr1,
		fee:        defaultGas,
		expInErr: []string{
			"failed to use fee grant",
			fmt.Sprintf("granter: %s", addr1),
			fmt.Sprintf("grantee: %s", addr4),
			fmt.Sprintf(`fee: "%s"`, defaultGasStr),
			`msgs: ["/testpb.TestMsg"]`,
			"fee-grant not found",
		},
	},
}

This refactoring improves the clarity and readability of the test cases by using more descriptive names and a consistent structure.

internal/handlers/msg_service_router.go (1)

Line range hint 103-103: Address the redundant error handling in registerMsgServiceHandler.

func (msr *PioMsgServiceRouter) registerMsgServiceHandler(sd *grpc.ServiceDesc, method grpc.MethodDesc, handler interface{}) error {
	// existing implementation
	return nil  // Remove this line as it's redundant and unreachable
}

This change removes the redundant return statement at the end of the function, which is unreachable due to the function's structure.

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 6b3638a and ee0b7cc.
Files selected for processing (7)
  • .github/workflows/test.yml (2 hunks)
  • internal/antewrapper/min_gas_prices_decorator_test.go (5 hunks)
  • internal/antewrapper/prov_feegrant_test.go (7 hunks)
  • internal/antewrapper/provenance_fee.go (1 hunks)
  • internal/antewrapper/testutil_test.go (3 hunks)
  • internal/handlers/msg_fee_invoker_test.go (6 hunks)
  • internal/handlers/msg_service_router.go (1 hunks)
Additional Context Used
GitHub Check Runs (1)
golangci-lint failure (11)

internal/handlers/msg_service_router.go: [failure] 103-103:
(*PioMsgServiceRouter).registerMsgServiceHandler - result 0 (error) is always nil (unparam)

Additional comments not posted (13)
.github/workflows/test.yml (1)

Line range hint 1-1: Ensure the workflow is correctly triggered for all relevant changes.

This script checks that the workflow is set up to trigger on changes to all relevant files, ensuring that tests and coverage reports are generated as expected.

Verification successful

The verification process has successfully identified the trigger conditions for the GitHub Actions workflow in the .github/workflows/test.yml file. The workflow is configured to trigger on both pull_request and push events, specifically for pushes to the main branch. Additionally, the workflow uses a pattern to ensure that it triggers for changes to .go files, go.mod, go.sum, and the workflow file itself.

Here is the summary of the findings:

  • The workflow triggers on pull_request and push events.
  • For push events, it specifically triggers for changes to the main branch.
  • The workflow is configured to check for changes in .go files, go.mod, go.sum, and the workflow file itself, ensuring that it triggers for all relevant changes.

This setup aligns with the initial review concern about ensuring the workflow triggers for all relevant changes. Therefore, the workflow configuration is verified to be correct.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that the workflow triggers correctly for changes in Go files, go.mod, go.sum, and the workflow file itself.

# Test: List all files that should trigger the workflow.
rg --type yaml --type go --type mod --type sum --files

Length of output: 84



Script:

#!/bin/bash
# Corrected script to verify that the workflow triggers correctly for changes in Go files, go.mod, go.sum, and the workflow file itself.

# Correct the file type for Go module files and rerun the test to list all files that should trigger the workflow.
rg --type yaml --type go --type gomod --type sum --files

# Additionally, check the workflow file to ensure it is set to trigger on changes to these files.
cat .github/workflows/test.yml | grep 'on:'

Length of output: 494



Script:

#!/bin/bash
# Revised script to verify that the workflow triggers correctly for changes in Go files, go.mod, go.sum, and the workflow file itself.

# Use a more general approach to list files with specific extensions.
find . -type f \( -name "*.go" -o -name "go.mod" -o -name "go.sum" -o -name "*.yml" \)

# Extract the actual trigger conditions from the workflow file.
cat .github/workflows/test.yml | grep -A 10 'on:'

Length of output: 30284

internal/antewrapper/min_gas_prices_decorator_test.go (8)

9-9: New import google.golang.org/protobuf/protoadapt added.

This import is necessary for the new functionality in FeeTxWrapper to adapt messages to the V2 protobuf format.


15-19: New imports for codec and app added.

These imports are required for the new codec.Codec field in FeeTxWrapper and for creating a test encoding configuration in TestAnteHandle.


54-55: Interface assertions for sdk.Tx and sdk.FeeTx on FeeTxWrapper.

These assertions ensure that FeeTxWrapper correctly implements the required interfaces.


60-60: Added Codec codec.Codec field to FeeTxWrapper.

This addition is crucial for handling codec operations within the FeeTxWrapper methods.


64-69: Implemented GetMsgsV2 method in FeeTxWrapper using protoadapt.

This method correctly adapts the messages to the V2 format using the newly added protoadapt package.


72-78: Added FeePayer and FeeGranter methods to FeeTxWrapper.

These methods are essential for identifying the payer and granter of fees, utilizing the codec for serialization.


80-86: New constructors NewFeeTxWrapper and NewFeeTx that include codec codec.Codec.

These constructors are updated to pass the codec, aligning with the new field in FeeTxWrapper.


Line range hint 89-279: Updated TestAnteHandle to use new FeeTxWrapper and handle different fee scenarios.

The test cases are well-structured and cover various scenarios, ensuring that the fee handling logic in FeeTxWrapper is robust.

internal/handlers/msg_fee_invoker_test.go (4)

13-21: Updated imports to reflect new package aliases and dependencies.

The changes in imports are consistent with the updates in the package structure and are necessary for the new functionalities.


187-187: Introduced a new transaction configuration emptyTxCfg for testing.

This configuration is used to test the behavior of the system with an intentionally bad decoder setup, which is a valid test scenario.


Line range hint 222-242: Updated createTestTx and createTestTxWithFeeGrant functions to handle new transaction logic.

These functions are correctly updated to reflect the new transaction handling logic, ensuring that the tests are aligned with the current system behavior.


Line range hint 303-340: Updated CreateTestTx to handle new signing logic.

The changes in the transaction creation logic are necessary to accommodate the new signing requirements and ensure that the system behaves as expected under different scenarios.

internal/antewrapper/provenance_fee.go Outdated Show resolved Hide resolved
@SpicyLemon SpicyLemon merged commit 6bda36f into main May 13, 2024
25 of 36 checks passed
@SpicyLemon SpicyLemon deleted the dwedul/1760-fix-internal-tests branch May 13, 2024 19:04
nullpointer0x00 pushed a commit that referenced this pull request May 15, 2024
* [1760]: Fix the min gas prices decorator tests.

* [1760]: In GetFeePayerUsingFeeGrant, treat the fee payer and granter as AccAddresses since they're now being given to us as byte slices which don't convert to strings properly in formats.

* [1760]: Fix the feegrant tests.

* [1760]: Fix signmode stuff in the testutil tests.

* [1760]: Fix signing stuff in the msg-fee invoker test.

* [1760]: Remove TODO about whether something was needed, it was, so I kept it.

* [1760]: Re-enable the internal/antewrapper tests in the github action.
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