ci(golangci-lint): add unparam linter #6263

acud · 2024-08-15T20:08:36Z

Motivation

Adds the unparam linter.

Description

Test Plan

TODO

Explain motivation or link existing issue(s)
Test changes and document test plan
Update documentation as needed
Update changelog as needed

codecov · 2024-08-16T19:38:23Z

Codecov Report

Attention: Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.1%. Comparing base (864aedb) to head (b7bb382).
Report is 5 commits behind head on develop.

Files	Patch %	Lines
api/grpcserver/v2alpha1/layer.go	80.0%	1 Missing ⚠️
node/node.go	50.0%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6263   +/-   ##
=======================================
  Coverage     82.0%   82.1%           
=======================================
  Files          307     307           
  Lines        34105   34101    -4     
=======================================
+ Hits         27992   28007   +15     
+ Misses        4334    4315   -19     
  Partials      1779    1779

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

poszu · 2024-08-19T06:37:51Z

atxsdata/warmup_test.go

@@ -17,11 +17,12 @@ import (
 	"github.com/spacemeshos/go-spacemesh/sql/mocks"
 )

+var nonce = types.VRFPostIndex(1)


How about inlining types.VRFPostIndex(1) in gatx() instead of creating a global?

Also ideally it should not be the same value every time 😅

poszu · 2024-08-19T06:39:04Z

beacon/handlers_test.go

+	// FIXME: pulled out to satisfy the linter but needs to be
+	// actually modified in tests to run different values.
+	epoch = types.EpochID(10)
+	round = types.RoundID(5)


Then perhaps add a //nolint with an explanation to the function definition instead?

poszu · 2024-08-19T08:37:20Z

genvm/vm_test.go

-func (t *tester) estimateDrainGas(principal, vault, to, amount int, nonce core.Nonce) int {
+const vaultVal = 20
+
+func (t *tester) estimateDrainGas(principal, to, amount int, nonce core.Nonce) int {


Is this the right way to fix it? Perhaps the tests should use different values?

poszu · 2024-08-19T08:42:51Z

hare3/protocol_test.go

+// FIXME: made to satisfy the linter but actually needs unit test
+// changes to test different cases.
+const iter1 = uint8(1)


How about using //nolint then?

poszu · 2024-08-19T08:44:46Z

hare3/hare_test.go

@@ -753,7 +753,9 @@ func TestHandler(t *testing.T) {
 	})
 }

-func gatx(id types.ATXID, epoch types.EpochID, smesher types.NodeID, base, height uint64) types.ActivationTx {
+const epoch = types.EpochID(1)


I'm not sure about this linter if enabling it ends up using globals. The flow of information is broken now and the code is more difficult to follow.

Also I believe epoch should in at least one test be a different value from the others - or are all these tests seriously only interested in the very first epoch after genesis? I.e. the one where nothing happens except ATXs are published?

fasmat · 2024-08-19T15:57:18Z

api/grpcserver/post_service_test.go

@@ -74,7 +73,6 @@ func launchPostSupervisor(
 func launchPostSupervisorTLS(


This and launchPostSupervisor are basically identical, except that launchPostSupervisor doesn't yet use sql.InMemoryTest() and this function has the wrong name for the logger of the post manager.

I think launchPostSupervisorTLS can be removed and in the one place where it is called be replaced with launchPostSupervisor

fasmat · 2024-08-19T15:58:41Z

atxsdata/warmup_test.go

@@ -17,11 +17,12 @@ import (
 	"github.com/spacemeshos/go-spacemesh/sql/mocks"
 )

+var nonce = types.VRFPostIndex(1)


Also ideally it should not be the same value every time 😅

fasmat · 2024-08-19T16:02:07Z

fetch/cache_test.go

+	hp, _ := cache.get(hash)
+	return hp


I feel like either here or where this helper is called an assertion for the boolean is missing (I believe it should always be true in the current tests).

fasmat · 2024-08-19T16:06:41Z

genvm/vm_test.go

@@ -1459,10 +1461,12 @@ func runTestCases(t *testing.T, tcs []templateTestCase, genTester func(t *testin
 	}
 }

-func testWallet(t *testing.T, defaultGasPrice int, template core.Address, genTester func(t *testing.T) *tester) {
+const defaultGasPrice = 1


The defaultGasPrice was 20 before, why the change to 1? Also should this maybe be just different values for different tests?

fasmat · 2024-08-19T16:09:08Z

tortoise/state.go

-func decodeVotes(evicted, blid types.LayerID, base *ballotInfo, exceptions types.Votes) (votes, types.LayerID, error) {
+func decodeVotes(blid types.LayerID, base *ballotInfo, exceptions types.Votes) (votes, types.LayerID, error) {


That seems suspicious, are we sure that evicted is not needed and just wasn't forgotten when implementing decodeVotes?

fasmat · 2024-08-19T16:10:20Z

systest/tests/smeshing_test.go

+const (
+	start = 8
+	end   = 20
+)


I feel like instead of making these constant the test above should just use different start and end layers for vesting. Wdyt?

fasmat · 2024-08-19T16:17:32Z

miner/active_set_generator_test.go

+func unixPtr(sec int64) *time.Time {
+	t := time.Unix(sec, 0)
 	return &t
 }


This function isn't needed in my opinion. epochStart can just be a value instead of a pointer and instead of checking it for nil one can call epochStart.IsZero() on it to see if it was set or not.

Wdyt?

fasmat · 2024-08-19T16:20:53Z

hare3/protocol_test.go

@@ -182,8 +186,10 @@ type setup struct {
 	proposals []types.ProposalID
 }

-func (s *setup) thresh(v uint16) *setup {
-	s.threshold = v
+const threshold = 10


Same as above, probably // nolint is the way to go and fix it at some later point.

fasmat · 2024-08-19T16:22:27Z

hare4/hare_test.go

@@ -928,7 +928,9 @@ func TestHandler(t *testing.T) {
 	})
 }

-func gatx(id types.ATXID, epoch types.EpochID, smesher types.NodeID, base, height uint64) types.ActivationTx {
+const epoch = types.EpochID(1)


Same as above, epoch 1 is different from all other epochs, we shouldn't only test for that specific. We should have at least a single test where epoch is a different value.

fasmat · 2024-08-19T16:24:17Z

hare4/protocol_test.go

-func (t *toutput) iter(i uint8) *toutput {
+// FIXME: made to satisfy the linter but actually needs unit test
+// changes to test different cases.
+const iter1 = uint8(1)


Just like in the other places, I believe we should have variable values for this in different tests and if we don't change it within this PR, then // nolint for now.

fasmat · 2024-08-19T16:24:25Z

hare4/protocol_test.go

@@ -182,8 +186,10 @@ type setup struct {
 	proposals []types.ProposalID
 }

-func (s *setup) thresh(v uint16) *setup {
-	s.threshold = v
+const threshold = 10


Same as above

fasmat · 2024-08-19T16:27:11Z

miner/proposal_builder_test.go

@@ -145,9 +147,10 @@ func expectMeshHash(hash types.Hash32) expectOpt {
 	}
 }

+const epochExpectCounter = types.EpochID(3)


This should probably always be epoch+1 and epoch should again vary between tests (or // nolint)

acud · 2024-08-19T16:57:56Z

Thanks for the review guys, but the intent here was to enable the linter, however, I think that addressing all this will make this PR huge and unreasonable to review. We should probably fix the underlying issues and then enable the linter. Thanks

fasmat · 2024-08-19T16:58:20Z

proposals/eligibility_validator_test.go

@@ -83,8 +84,10 @@ func geligibilities(js ...uint32) (rst []types.VotingEligibility) {
 	return rst
 }

+const j = uint32(1)


This also feels like it should vary between tests

fasmat · 2024-08-19T16:58:51Z

sql/transactions/transactions_test.go

@@ -17,11 +17,13 @@ import (
 	"github.com/spacemeshos/go-spacemesh/sql/transactions"
 )

+const amount = uint64(191)


I think this should vary between tests.

fasmat · 2024-08-19T16:59:31Z

tortoise/fixture_test.go

@@ -125,12 +127,14 @@ type bopt struct {
 	opts []ballotOpt
 }

-func (b *bopt) beacon(value string) *bopt {
+const beaconVal = "a"


This can probably be inlined

fasmat · 2024-08-19T17:00:01Z

tortoise/fixture_test.go

@@ -77,9 +77,11 @@ func (s *smesher) rawatx(id types.ATXID, publish types.EpochID, opts ...*aopt) *
 	return val
 }

+const publish = 1


As stated before, epoch 1 is special different from all other epochs, we should have at least one test were we have a different publish epoch.

acud force-pushed the add-unparam branch 2 times, most recently from 3ec0285 to d95fe07 Compare August 16, 2024 19:23

acud force-pushed the add-unparam branch from d95fe07 to 64bd1fd Compare August 16, 2024 20:02

build(golangci): add unparam linter

b7bb382

acud force-pushed the add-unparam branch from 64bd1fd to b7bb382 Compare August 16, 2024 20:22

acud marked this pull request as ready for review August 16, 2024 21:16

acud requested review from dshulyak, fasmat, poszu and ivan4th as code owners August 16, 2024 21:16

poszu reviewed Aug 19, 2024

View reviewed changes

fasmat requested changes Aug 19, 2024

View reviewed changes

fasmat reviewed Aug 19, 2024

View reviewed changes

acud closed this Aug 19, 2024

acud deleted the add-unparam branch August 19, 2024 16:58

fasmat reviewed Aug 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci(golangci-lint): add unparam linter #6263

ci(golangci-lint): add unparam linter #6263

acud commented Aug 15, 2024

codecov bot commented Aug 16, 2024 •

edited

Loading

poszu Aug 19, 2024

fasmat Aug 19, 2024

poszu Aug 19, 2024

poszu Aug 19, 2024

poszu Aug 19, 2024

poszu Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

acud commented Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

fasmat Aug 19, 2024

		@@ -74,7 +73,6 @@ func launchPostSupervisor(
		func launchPostSupervisorTLS(

		func decodeVotes(evicted, blid types.LayerID, base *ballotInfo, exceptions types.Votes) (votes, types.LayerID, error) {
		func decodeVotes(blid types.LayerID, base *ballotInfo, exceptions types.Votes) (votes, types.LayerID, error) {

ci(golangci-lint): add unparam linter #6263

ci(golangci-lint): add unparam linter #6263

Conversation

acud commented Aug 15, 2024

Motivation

Description

Test Plan

TODO

codecov bot commented Aug 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acud commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2024 •

edited

Loading