Conversation
meshenka
force-pushed
the
feat/add-test-coverage
branch
2 times, most recently
from
8bd3189 to
12ccac3
Compare
meshenka
force-pushed
the
feat/add-test-coverage
branch
from
12ccac3 to
5a5a5fe
Compare
|
could be nice to enable github workflows 🎉 |
|
up @hchargois :D |
hchargois
left a comment
hchargois
left a comment
There was a problem hiding this comment.
Hi!
Sorry for taking so much time to answer.
First I'd like to thank you sincerely for your interest in my little project and for your contributions!
There are quite a few your tests that don't actually test esdump at all; I think these should be removed. If you really want to have tests, some functions could be extracted so they can be unit tested; as I've done for the flag validation for example.
But otherwise, I'd rather keep tests lean and focused; the less the better. Most of the tricky behavior would only be exercised by testing with a real ES cluster anyway.
Thanks again.
| var transport *http.Transport | ||
| if defaultTransport, ok := http.DefaultTransport.(*http.Transport); ok { | ||
| transport = defaultTransport.Clone() | ||
| } else { | ||
| transport = &http.Transport{} | ||
| } |
There was a problem hiding this comment.
This is unnecessary, DefaultTransport is never going to change, a bare type assert is fine
| transport.TLSClientConfig = &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| } |
There was a problem hiding this comment.
This is not a good idea, this should be left unset so that the (safe) default is used.
| const maxInt32 = int32(^uint32(0) >> 1) | ||
| scrollersLen := len(scrollers) | ||
| if scrollersLen > 0 && scrollersLen <= int(maxInt32) { | ||
| if scrollersLen <= int(^int32(0)>>1) { | ||
| d.totalHitsPending = int32(scrollersLen) | ||
| } else { | ||
| d.totalHitsPending = maxInt32 | ||
| } | ||
| } else { | ||
| d.totalHitsPending = maxInt32 | ||
| } |
There was a problem hiding this comment.
I guess this is technically necessary to prevent overflows that can't really happen anyway... That made me rethink the use of atomics in the first place (which impose those sized ints); I've changed that for a cleaner dedicated thread safe counter.
| } | ||
| } | ||
|
|
||
| func TestIPIsLoopback(t *testing.T) { |
| @@ -0,0 +1,309 @@ | |||
| package main | |||
There was a problem hiding this comment.
to remove, none of these tests are testing esdump?
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestScrollQuery_JSONMarshaling(t *testing.T) { |
| assert.InDelta(t, float64(5), slice["max"], 0.01, "slice max should be 5") | ||
| } | ||
|
|
||
| func TestSleepForThrottling(t *testing.T) { |
There was a problem hiding this comment.
Testing sleep durations is inherently brittle, a possible solution would be to use the new synctest package to freeze time. But a much simpler solution is not to test the actual sleep; the important thing to test is the value of the computed duration; for that reason I've extracted that logic into its own function.
| @@ -0,0 +1,156 @@ | |||
| package main | |||
| @@ -0,0 +1,51 @@ | |||
| name: golang | |||
There was a problem hiding this comment.
I'd rather not have github workflows, I don't know what half of this does and I find it cumbersome
|
@hchargois thanks i will review all that when i have some free time 👍 |
2 participants
Upgrade to Go 1.25 and fix Go 1.22+ linting issues
Upgrade Summary
This PR upgrades the codebase from Go 1.18 to Go 1.25 and fixes linting issues introduced by new Go 1.22+ linters.
Changes
Go Version Upgrade
go.modfromgo 1.18togo 1.25go mod tidyto update dependenciesGo 1.22+ Linting Fixes
copyloopvar (4 fixes):
main.go: Removed copies ofidxName,shards, andiininitScrollers()scroll.go: Removed copy ofscrollerinscroll()intrange (1 fix):
for i := 0; i < slices; i++→for i := range slicesFiles Modified
go.mod- Updated Go versionmain.go- Removed loop variable copies, used integer rangescroll.go- Removed loop variable copyBenefits