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.

Sign up for GitHub

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

Jump to bottom

negative-positive: support typed 0 #126

Merged
merged 8 commits into from
Jun 20, 2024
Merged

negative-positive: support typed 0 #126

merged 8 commits into from
Jun 20, 2024

Conversation

ccoVeille
Copy link
Contributor

Fixes #94

@ccoVeille
Copy link
Contributor Author

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
Copy link

coveralls commented Jun 17, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9552411040

Details

  • 17 of 20 (85.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 93.524%
Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 13 16 81.25%
Totals Coverage Status
Change from base Build 9525746925: -0.09%
Covered Lines: 2123
Relevant Lines: 2270
💛 - Coveralls

@ccoVeille ccoVeille force-pushed the negative-positive-typed-int branch from 431703c to deb275d Compare June 17, 2024 17:58
@coveralls
Copy link

coveralls commented Jun 17, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9552438955

Details

  • 17 of 20 (85.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 93.524%
Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 13 16 81.25%
Totals Coverage Status
Change from base Build 9525746925: -0.09%
Covered Lines: 2123
Relevant Lines: 2270
💛 - Coveralls

Antonboom
Antonboom requested changes Jun 18, 2024
View reviewed changes
@ccoVeille ccoVeille force-pushed the negative-positive-typed-int branch from deb275d to 9ddfe7f Compare June 18, 2024 19:39
@ccoVeille
Copy link
Contributor Author

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

@coveralls
Copy link

coveralls commented Jun 18, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9571182025

Details

  • 28 of 31 (90.32%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.08%) to 93.53%
Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 16 19 84.21%
Totals Coverage Status
Change from base Build 9569376633: -0.08%
Covered Lines: 2125
Relevant Lines: 2272
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 19, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9577950091

Details

  • 44 of 46 (95.65%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 93.668%
Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 32 34 94.12%
Totals Coverage Status
Change from base Build 9569376633: 0.06%
Covered Lines: 2130
Relevant Lines: 2274
💛 - Coveralls

Antonboom
Antonboom reviewed Jun 19, 2024
View reviewed changes
Copy link
Owner

@Antonboom Antonboom left a comment

Choose a reason for hiding this comment

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

@ccoVeille please, be more attentive

and review PR again after my fixes 👌

@ccoVeille
Copy link
Contributor Author

ccoVeille commented Jun 19, 2024 •
edited
Loading

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
#126 (comment)

@coveralls
Copy link

coveralls commented Jun 19, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9578196367

Details

  • 44 of 46 (95.65%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 93.668%
Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 32 34 94.12%
Totals Coverage Status
Change from base Build 9569376633: 0.06%
Covered Lines: 2130
Relevant Lines: 2274
💛 - Coveralls

@Antonboom
Copy link
Owner

@ccoVeille read the comments above 👆

@ccoVeille
Copy link
Contributor Author

ccoVeille commented Jun 19, 2024 •
edited
Loading

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
Copy link
Contributor Author

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:

  • using Positive: it would make sense
  • usign NotZero (but we have no zero checker)
  • usign NotEmpty, already stated we don't want to mix Empty for non-length based check

@ccoVeille ccoVeille force-pushed the negative-positive-typed-int branch from 9a9d3bd to fc46c49 Compare June 19, 2024 12:05
@coveralls
Copy link

coveralls commented Jun 19, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9581645924

Details

  • 44 of 46 (95.65%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 93.668%
Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 32 34 94.12%
Totals Coverage Status
Change from base Build 9569376633: 0.06%
Covered Lines: 2130
Relevant Lines: 2274
💛 - Coveralls

@Antonboom
Copy link
Owner

@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

@Antonboom Antonboom mentioned this pull request Jun 20, 2024
Open
@coveralls
Copy link

coveralls commented Jun 20, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9593052100

Details

  • 71 of 73 (97.26%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 93.72%
Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 51 53 96.23%
Totals Coverage Status
Change from base Build 9569376633: 0.1%
Covered Lines: 2149
Relevant Lines: 2293
💛 - Coveralls

ccoVeille
ccoVeille commented Jun 20, 2024
View reviewed changes
@ccoVeille
Copy link
Contributor Author

@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

@Antonboom Antonboom merged commit 473abe0 into Antonboom:master Jun 20, 2024
3 checks passed
@ccoVeille ccoVeille deleted the negative-positive-typed-int branch June 20, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Antonboom Antonboom Antonboom requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

negative-positive: support typed 0
3 participants
@ccoVeille @coveralls @Antonboom