chore: enable `errcheck` linter #16406

ruthishvitwit · 2023-06-02T10:14:57Z

Description

The aim of this pr is to solve and fix all the errors after enabling Errcheck.I have made some of changes in a way i could understand the issue.Please suggest any changes or pointout any errors

Closes: #16293

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

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)

baseapp/utils_test.go

crypto/keys/secp256k1/internal/secp256k1/secp256_test.go

x/auth/client/cli/tx_sign.go

x/distribution/keeper/msg_server.go

x/staking/testutil/helpers.go

atheeshp

Seems like omitting the errors at many places and also pending @julienrbrt 's comments.

crypto/keys/secp256k1/internal/secp256k1/secp256_test.go

likhita-809

same comments as julien and atheesh
Can you also resolve the conflicts?

julienrbrt

lgtm! thank you @ruthishvitwit!

…le-errcheck

faddat · 2023-07-05T09:53:31Z

Hey thanks @ruthishvitwit this is one of the linters that helps me sleep at night

atheeshp

mostly lgtm, a few nits.

baseapp/utils_test.go

testutil/network/network.go

x/feegrant/keeper/keeper.go

x/nft/simulation/operations_test.go

likhita-809

other than atheesh's comments and few nits, mostly looks good.
There are also some failing tests. Can you please take a look at them ?

baseapp/abci_test.go

client/v2/autocli/common_test.go

julienrbrt · 2023-07-06T14:24:25Z

@ruthishvitwit, could you fix the conflict and implement the last comments from Atheesh, so we can merge?

…le-errcheck

julienrbrt

Two tests are failing, could you have a look?

julienrbrt · 2023-07-07T06:09:22Z

x/gov/keeper/proposal_test.go

-		suite.govKeeper.SetProposal(suite.ctx, proposal)
+		err = suite.govKeeper.SetProposal(suite.ctx, proposal)
+		suite.Require().NoError(err)
+		err = suite.govKeeper.SetProposal(suite.ctx, proposal)


Why is this duplicated?

julienrbrt · 2023-07-07T06:09:31Z

x/gov/keeper/proposal_test.go

+		err = suite.govKeeper.SetProposal(suite.ctx, proposal)
+		suite.Require().NoError(err)
+
+		err = suite.govKeeper.SetProposal(suite.ctx, proposal)


ruthishvitwit · 2023-07-10T06:06:00Z

@julienrbrt I am not able to solve the errors in the tests which are failing . Will u be able to take a look at it once??

…smos-sdk into enable-errcheck

julienrbrt

For the first test failing it is fine to do not check the error.
For the second TestRollback, we should ensure there are validators, so you need to update the genesis (app.DefaultGenesis) and add some validators (e.g.

cosmos-sdk/simapp/test_helpers.go

Line 76 in 4c6096c

    
           genesisState, err = simtestutil.GenesisStateWithValSet(app.AppCodec(), genesisState, valSet, []authtypes.GenesisAccount{acc}, balance)

)

julienrbrt · 2023-07-10T13:22:07Z

tests/e2e/staking/suite.go

@@ -153,4 +153,5 @@ func (s *E2ETestSuite) TestBlockResults() {

 		return nil
 	}, 10)
+	require.NoError(err)


This is ok to ignore, as it may fail as the first few blocks, and this is why we retry for 10 blocks.
This will "fix" the test issue you have.

…le-errcheck

julienrbrt · 2023-07-11T13:08:29Z

Can you fix the conflicts and let's merge!

…le-errcheck

(cherry picked from commit fd7e549) # Conflicts: # .golangci.yml # simapp/internal/testnet/cometrpc_test.go # simapp/internal/testnet/cometstarter_test.go # simapp/internal/testnet/example_basic_test.go # tests/integration/distribution/keeper/msg_server_test.go # tests/integration/gov/keeper/keeper_test.go # tests/integration/slashing/keeper/keeper_test.go # x/distribution/simulation/operations_test.go # x/slashing/keeper/genesis_test.go # x/slashing/keeper/keeper_test.go # x/slashing/simulation/operations_test.go

Fix: Errcheck enabled

26057da

ruthishvitwit requested a review from a team as a code owner June 2, 2023 10:14

github-actions bot added C:CLI C:Rosetta Issues and PR related to Rosetta C:Simulations C:x/auth C:x/authz C:x/bank C:x/distribution distribution module related C:x/genutil genutil module issues C:x/gov C:x/group C:x/mint C:x/params C:x/slashing C:x/staking Type: CI labels Jun 2, 2023

github-prbot requested review from a team, julienrbrt and samricotta and removed request for a team June 2, 2023 10:15

ruthishvitwit changed the title ~~Fix: Errcheck enabled~~ Chore: Errcheck enabled Jun 2, 2023

julienrbrt changed the title ~~Chore: Errcheck enabled~~ chore: enabled errcheck linter Jun 2, 2023

julienrbrt changed the title ~~chore: enabled errcheck linter~~ chore: enable errcheck linter Jun 2, 2023

julienrbrt requested changes Jun 2, 2023

View reviewed changes

tac0turtle assigned julienrbrt, samricotta and likhita-809 Jun 3, 2023

atheeshp reviewed Jun 5, 2023

View reviewed changes

crypto/keys/secp256k1/internal/secp256k1/secp256_test.go Outdated Show resolved Hide resolved

likhita-809 reviewed Jun 6, 2023

View reviewed changes

julienrbrt approved these changes Jul 5, 2023

View reviewed changes

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into enab…

f8fb404

…le-errcheck

new merge lint fixes

9572f3f

atheeshp approved these changes Jul 5, 2023

View reviewed changes

likhita-809 reviewed Jul 5, 2023

View reviewed changes

baseapp/abci_test.go Outdated Show resolved Hide resolved

client/v2/autocli/common_test.go Outdated Show resolved Hide resolved

final changes

b81b2a6

ruthishvitwit added 2 commits July 7, 2023 10:30

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into enab…

62fa4fe

…le-errcheck

made final changes after resolving conflicts

fd3e177

julienrbrt reviewed Jul 7, 2023

View reviewed changes

Merge branch 'main' into enable-errcheck

7a6a70d

ruthishvitwit added 2 commits July 10, 2023 11:36

removed duplicate lines

e200155

Merge branch 'enable-errcheck' of https://github.com/ruthishvitwit/co…

27d50f8

…smos-sdk into enable-errcheck

julienrbrt reviewed Jul 10, 2023

View reviewed changes

ruthishvitwit added 2 commits July 11, 2023 14:39

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into enab…

3f88d2e

…le-errcheck

removed some new errchecks after merge

6450fc0

ruthishvitwit requested a review from julienrbrt July 11, 2023 09:56

julienrbrt approved these changes Jul 11, 2023

View reviewed changes

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into enab…

d19a928

…le-errcheck

julienrbrt enabled auto-merge July 12, 2023 08:57

julienrbrt added this pull request to the merge queue Jul 12, 2023

Merged via the queue into cosmos:main with commit fd7e549 Jul 12, 2023

mergify bot mentioned this pull request Jul 12, 2023

chore: enable errcheck linter (backport #16406) #16946

Closed

julienrbrt removed the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Jul 12, 2023

170210 mentioned this pull request Jan 15, 2024

Triage finschia-sdk#782 Finschia/cosmos-sdk#70

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: enable `errcheck` linter #16406

chore: enable `errcheck` linter #16406

ruthishvitwit commented Jun 2, 2023

atheeshp left a comment

likhita-809 left a comment

julienrbrt left a comment

faddat commented Jul 5, 2023

atheeshp left a comment

likhita-809 left a comment

julienrbrt commented Jul 6, 2023

julienrbrt left a comment

julienrbrt Jul 7, 2023

julienrbrt Jul 7, 2023

ruthishvitwit commented Jul 10, 2023

julienrbrt left a comment

julienrbrt Jul 10, 2023

julienrbrt commented Jul 11, 2023

chore: enable errcheck linter #16406

chore: enable errcheck linter #16406

Conversation

ruthishvitwit commented Jun 2, 2023

Description

Author Checklist

Reviewers Checklist

atheeshp left a comment

Choose a reason for hiding this comment

likhita-809 left a comment

Choose a reason for hiding this comment

julienrbrt left a comment

Choose a reason for hiding this comment

faddat commented Jul 5, 2023

atheeshp left a comment

Choose a reason for hiding this comment

likhita-809 left a comment

Choose a reason for hiding this comment

julienrbrt commented Jul 6, 2023

julienrbrt left a comment

Choose a reason for hiding this comment

julienrbrt Jul 7, 2023

Choose a reason for hiding this comment

julienrbrt Jul 7, 2023

Choose a reason for hiding this comment

ruthishvitwit commented Jul 10, 2023

julienrbrt left a comment

Choose a reason for hiding this comment

julienrbrt Jul 10, 2023

Choose a reason for hiding this comment

julienrbrt commented Jul 11, 2023

chore: enable `errcheck` linter #16406

chore: enable `errcheck` linter #16406