Handlers - Embed small HTTP test fixtures#5001
Conversation
MuneebUllahKhan222
left a comment
MuneebUllahKhan222
left a comment
There was a problem hiding this comment.
LGTM! Just a couple of comments that I think we should address to make this more future-proof.
|
|
||
| // Fetched over HTTP; the ~18 MB upstream fixture would substantially grow pkg/handlers/testdata/ if embedded. | ||
| func TestHandleLargeHTTPJson(t *testing.T) { | ||
| resp, err := http.Get("https://raw.githubusercontent.com/ahrav/nothing-to-see-here/main/md_random_data.json.zip") |
There was a problem hiding this comment.
It’s fine to fetch these files over HTTP since they are quite large to keep in the codebase. However, we should consider moving them under a repository owned by the TruffleHog org so that we still retain access to them in case the original author deletes their repository in the future.
There was a problem hiding this comment.
Great suggestion Muneeb. Thanks
There was a problem hiding this comment.
Opened trufflesecurity/trufflehog-test-assets#1 to host these under the org. Once it merges I'll repoint the test URLs there in a small follow-up PR.
|
|
||
| // Fetched over HTTP; the ~5 MB upstream fixture would notably grow pkg/handlers/testdata/ if embedded. | ||
| func TestHandleHTTPJson(t *testing.T) { | ||
| resp, err := http.Get("https://raw.githubusercontent.com/ahrav/nothing-to-see-here/main/sm_random_data.json") |
There was a problem hiding this comment.
We should download and move this file to a repository under truffle hog's GH org.
MuneebUllahKhan222
added
the
review/product-eng
rosecodym
left a comment
rosecodym
left a comment
There was a problem hiding this comment.
Thanks for the housekeeping!
3 participants
Summary
Addresses the
// TODO: Embed a zip without making an HTTP request.at the formerpkg/handlers/handlers_test.go:42and opportunistically converts two adjacent tests that share the same upstream fixture.Embedded into
pkg/handlers/testdata/:aws-canary-creds.zipTestHandleFilesm.zipTestHandleHTTPJsonZip,BenchmarkHandleHTTPJsonZipTotal committed: ~2 MB, comparable to existing fixtures
test.debandtest.rpm(~1 MB each).Left HTTP-fetched with explanatory comments:
TestHandleHTTPJson(upstream fixture is ~5 MB)TestHandleLargeHTTPJson(upstream fixture is ~18 MB)Embedding these would notably grow
pkg/handlers/testdata/. Generating equivalent deterministic fixtures programmatically is a possible follow-up; happy to take direction from the team on which approach is preferred.Test plan
go vet ./pkg/handlers/...TestHandleFilepasses with the embedded fixtureTestHandleHTTPJsonZippasses with the embedded fixturego test ./pkg/handlers/...passes (~10 s)Checklist:
make test-community)?make lintthis requires golangci-lint)?Note
Low Risk
Test-only changes with no production code paths; slightly increases repo size via committed fixtures.
Overview
Replaces network-dependent handler tests with
go:embedfixtures underpkg/handlers/testdata/, soTestHandleFile,TestHandleHTTPJsonZip, andBenchmarkHandleHTTPJsonZipread frombytes.NewReaderinstead ofhttp.Get.Adds embedded
aws-canary-creds.zip(~308 B) andsm.zip(~1.8 MB).TestHandleHTTPJsonandTestHandleLargeHTTPJsonstill fetch over HTTP; comments note that embedding their ~5 MB and ~18 MB upstream assets would bloattestdata/.Reviewed by Cursor Bugbot for commit a3891b3. Bugbot is set up for automated code reviews on this repo. Configure here.