Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #341 +/- ##
==========================================
- Coverage 84.49% 84.44% -0.04%
==========================================
Files 10 10
Lines 4280 4280
==========================================
- Hits 3616 3614 -2
- Misses 406 407 +1
- Partials 258 259 +1 ☔ View full report in Codecov by Sentry. |
|
@lni What about the staticcheck errors should I fix them as well (they seem all to be related to bump of Go version rather than to core of this PR) |
|
@coufalja thanks for picking this up :) |
|
I have fixed the static-check issues in a separate commit. |
|
@lni Any feedback? |
|
It looks that meanwhile 1.1.0 was published, should I update this? It again breaks the lni/vfs so we will need to go through the full loop again. |
There was a problem hiding this comment.
Firstly, I applaud the premise of this PR.
Pinning to a tagged version of Pebble (v1.0.0) is much better than pinning to an untagged version (v0.0.0).
However, this PR could be smaller if you remove the refactors:
- Remove the
crypto/randstuff (not necessary) - Remove the explicit blank assignments
These refactors have nothing to do with pebble and should be proposed in a separate PR.
I suggest you close this PR and create a new, clean PR for Pebble 1.1.0 without any refactoring.
| b.SetBytes(int64(sz)) | ||
| input := make([]byte, sz) | ||
| rand.Read(input) | ||
| _, _ = rand.Read(input) |
There was a problem hiding this comment.
Why would a test require crypto/rand?
Is math/rand not good enough?
There was a problem hiding this comment.
Fixing lint issue. Like I would be fine with math but that would require changing the linter config which I felt I shouldn't touch.
| key.SetEntryKey(100, 1, i) | ||
| data := make([]byte, 64) | ||
| rand.Read(data) | ||
| _, _ = rand.Read(data) |
There was a problem hiding this comment.
If you aren't going to use the values, it is more idiomatic to omit the assignment rather than forcing explicit blank assignment.
rand.Read(data)See Golang std lib source code where unaccessed values do not use blank assignment.
https://github.com/golang/go/blob/master/src/crypto/rand/rand_test.go#L27-L28
| ) | ||
|
|
||
| func init() { | ||
| rand.Seed(time.Now().UnixNano()) |
There was a problem hiding this comment.
I agree that seeding the global RNG with a random value in a test suite is not a good idea.
Though neither is writing tests that depend on the determinism of a global RNG so 🤷♂️.
Is there any good reason that this needs to change?
Edit: I see the static check failures on @tylerwilliams PR #339 regarding deprecation of math/rand.
Looks good to me, but I recommend proposing this cleanup in a separate PR.
There was a problem hiding this comment.
Yeah I did a split into the separate commit given the CI is not passing. I could revert the commit with the rand changes. Actually I think that the lint failures was an oversight from previous merge bumping to Go 1.20.
4 participants
This is based off work of @tylerwilliams in #339 as I am eagerly waiting for this to be merged, I took the liberty to rebase the PR and open it again.