search

Antonboom / testifylint

The Golang linter that checks usage of github.com/stretchr/testify.
https://github.com/stretchr/testify
MIT License
101 stars 8 forks source link

negative-positive: support typed 0 #126

Closed ccoVeille closed 5 months ago

ccoVeille commented 5 months ago

Fixes #94

ccoVeille commented 5 months ago

I could iterate and make the commit atomic, so it won't be a block like this.

I created a isZeroValue separated from isZero to avoid conflicting with code that is already using isZero or isIntNumber

Tell me what you think about it @Antonboom

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 9552411040

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 13 16 81.25%
<!-- Total: 17 20 85.0% -->
Totals Coverage Status
Change from base Build 9525746925: -0.09%
Covered Lines: 2123
Relevant Lines: 2270
💛 - Coveralls
coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 9552438955

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 13 16 81.25%
<!-- Total: 17 20 85.0% -->
Totals Coverage Status
Change from base Build 9525746925: -0.09%
Covered Lines: 2123
Relevant Lines: 2270
💛 - Coveralls
ccoVeille commented 5 months ago

I made the changes. Let me know if you are fine with them

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 9571182025

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 16 19 84.21%
<!-- Total: 28 31 90.32% -->
Totals Coverage Status
Change from base Build 9569376633: -0.08%
Covered Lines: 2125
Relevant Lines: 2272
💛 - Coveralls
coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 9577950091

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 32 34 94.12%
<!-- Total: 44 46 95.65% -->
Totals Coverage Status
Change from base Build 9569376633: 0.06%
Covered Lines: 2130
Relevant Lines: 2274
💛 - Coveralls
ccoVeille commented 5 months ago

Thanks for the test refactoring.

Also the changes to generate and test one checker without the others 🥰

~I don't get the point of removing the unsigned int? I'm OK with it, but I'm unsure why.~

Edited: I found the reply here https://github.com/Antonboom/testifylint/pull/126#discussion_r1645601601

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 9578196367

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 32 34 94.12%
<!-- Total: 44 46 95.65% -->
Totals Coverage Status
Change from base Build 9569376633: 0.06%
Covered Lines: 2130
Relevant Lines: 2274
💛 - Coveralls
Antonboom commented 5 months ago

@ccoVeille read the comments above 👆

ccoVeille commented 5 months ago

I'm OK with the changes, you removed the uint vs negative, which is obvious

For records, I found examples of code where uint is used in something that should be converted to Positive

https://github.com/search?q=language%3Ago+%22assert.Less%28t%2C+uint%22&type=code https://github.com/search?q=language%3Ago+%22assert.Greater%28t%2C+uint%22&type=code

https://github.com/mysteriumnetwork/node/blob/adb67589d42929b8ad6f3a76177d585e4f96b7e4/consumer/entertainment/estimator_test.go#L45

assert.Less(t, uint64(0), e.VideoMinutes)

https://github.com/mehrdadrad/tcpprobe/blob/782fdd0b0962d91495e3ec02341f8d5c8915ea2e/tp_test.go#L61-L62

    assert.Less(t, uint32(0), c.stats.Rto)
    assert.Less(t, uint32(0), c.stats.Ato)

https://github.com/yl2chen/cidranger/blob/d1cb2c52f37ac257db665ddfaf4af22e89863471/trie_test.go#L545C1-L545C41

    assert.Less(t, uint64(0), baseLineHeap)

https://github.com/CoreumFoundation/coreum/blob/11801f3538029fa10ee270940c53106a75097c42/integration-tests/modules/wasm_test.go#L425

    assert.Greater(t, uint64(result.GasUsed), minGasExpected)

https://github.com/EpiK-Protocol/go-epik-actors/blob/113a169fc34421d9e075fd0057b541b94bd75609/actors/test/terminate_sectors_scenario_test.go#L140

        assert.Greater(t, uint64(state.LastUpdatedEpoch), uint64(0))

These are edge cases, but they are legitimate according to me.

But we could move them to a separate issue, and current PR could be merged like this.

ccoVeille commented 5 months ago

There is also something that is left aside

https://github.com/eosspark/geos/blob/e0ee87b6fbcc128bfc3230487cc4f329b7bff719/unittests/eosio_system_test.go#L1222

        assert.True(t, uint64(0) < prod["last_claim_time"].(uint64))

Some other can be found with such query https://github.com/search?q=language%3Ago+%22assert.True%28t%2C+uint%22&type=code

But here we could say it's open for debate, should we consider suggesting:

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 9581645924

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 32 34 94.12%
<!-- Total: 44 46 95.65% -->
Totals Coverage Status
Change from base Build 9569376633: 0.06%
Covered Lines: 2130
Relevant Lines: 2274
💛 - Coveralls
Antonboom commented 5 months ago

@ccoVeille

But, unsigned int can be positive, so they leads to compare they are not zero/empty.

this is just my mistake. I returned back uint support for assert.Positive and added real-life examples to tests

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 9593052100

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 51 53 96.23%
<!-- Total: 71 73 97.26% -->
Totals Coverage Status
Change from base Build 9569376633: 0.1%
Covered Lines: 2149
Relevant Lines: 2293
💛 - Coveralls
ccoVeille commented 5 months ago

@ccoVeille

But, unsigned int can be positive, so they leads to compare they are not zero/empty.

this is just my mistake. I returned back uint support for assert.Positive and added real-life examples to tests

I think we are good