Skip to content

feat: add capture-on-miss-flag#1229

Merged
tommysitu merged 2 commits into
SpectoLabs:masterfrom
schthms:add-capture-on-miss-flag
May 31, 2026
Merged

feat: add capture-on-miss-flag#1229
tommysitu merged 2 commits into
SpectoLabs:masterfrom
schthms:add-capture-on-miss-flag

Conversation

@schthms
Copy link
Copy Markdown
Contributor

@schthms schthms commented May 28, 2026

Adds a -capture-on-miss CLI flag that, when combined with -spy, configures spy mode to save request/response pairs to the simulation store on cache misses. Previously this behavior was only configurable via the admin API after startup.

Also adds unit tests covering the CaptureOnMiss path in spy mode and the SetModeWithArguments wiring for the new argument.

@tommysitu
Copy link
Copy Markdown
Member

@claude

@claude
Copy link
Copy Markdown

claude Bot commented May 28, 2026

Claude finished @tommysitu's task in 2m 34s —— View job


Code Review

  • Read changed files and PR diff
  • Review implementation correctness
  • Check test coverage
  • Post review findings

The implementation is clean and follows existing patterns well. A few issues to address:


Bug: Silent ignore when -capture-on-miss is used without -spy

core/cmd/hoverfly/main.go:525

if *spy && *captureOnMiss {
    ...
}

If a user passes -capture-on-miss without -spy, the flag is silently ignored — no warning, no error. This is a footgun. Either add a fatal error (consistent with how the other mode conflict checks work in getInitialMode), or at minimum a warning log:

if *captureOnMiss && !*spy {
    log.Fatal("-capture-on-miss can only be used with -spy mode")
}

Fix this →


Timing: SetModeWithArguments fires before simulation import

core/cmd/hoverfly/main.go:525 vs main.go:574

SetModeWithArguments calls hf.CacheMatcher.PreloadCache(hf.Simulation) internally, but simulations aren't imported until later (hoverfly.Import(ev) at line 574+). For users who combine -capture-on-miss -spy -import path/to/sim.json, the cache will be preloaded against an empty simulation at startup.

The bigger concern is whether simulation import refreshes the cache — if it does, this is harmless; if it doesn't, matched requests won't benefit from the cache. Worth verifying the import code path also calls PreloadCache.


Minor: Missing assertion on savedArguments in test

core/modes/spy_mode_test.go:192Test_SpyMode_OnCacheMiss_WhenCaptureOnMissEnabled_SavesRequestAndResponse

The test verifies savedRequest and savedResponse are populated, but doesn't assert stub.savedArguments.CaptureOnMiss == true. Adding this would make the test more complete.

Expect(stub.savedArguments).ToNot(BeNil())
Expect(stub.savedArguments.CaptureOnMiss).To(BeTrue())

Fix this →


No functional test for the new CLI flag

There are no functional tests covering -capture-on-miss as a startup flag (in functional-tests/core/). The existing unit tests cover the mode's Process path and SetModeWithArguments, but not the end-to-end startup wiring. Given this is a new CLI flag, a functional test would help catch regressions.


What's Good

  • Pointer receiver refactor for hoverflySpyStub is correct and necessary to allow state tracking across test method calls.
  • GetResponseBody properly restores response.Body after reading it — no double-drain bug.
  • Both positive and negative cases are tested (CaptureOnMiss: true saves, CaptureOnMiss: false does not).
  • The implementation correctly threads CaptureOnMiss from the view through SetArguments into ModeArguments.
  • Error handling in spy_mode.go on save failure is consistent with the existing error-handling pattern.

@tommysitu
Copy link
Copy Markdown
Member

Hi @schthms, the change is sensible, just some minor issues spotted by claude which you might want to address as well.

@schthms
Copy link
Copy Markdown
Contributor Author

schthms commented May 29, 2026

Thanks @tommysitu. I addressed the review comments:

  • Moved the -capture-on-miss wiring to after all imports complete. SetModeWithArguments calls PreloadCache internally, so placing it before imports meant the cache was built against an empty simulation when using HOVERFLY_IMPORT_RECORDS. The simulation is now fully loaded before the cache is preloaded in all cases.
  • Added a fatal error when -capture-on-miss is passed without -spy so it fails loudly instead of silently doing nothing.
  • Strengthened the CaptureOnMiss unit test with an assertion on savedArguments to verify the arguments actually reach the Save call.
  • Added functional tests for the capture-on-miss path: one verifying a cache miss saves the pair, one verifying a cache hit does not.

Copy link
Copy Markdown
Member

@tommysitu tommysitu left a comment

Choose a reason for hiding this comment

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

Adding new flag to hoverfly cli, no-ops for existing feature. LGTM

@tommysitu tommysitu merged commit 9fed32c into SpectoLabs:master May 31, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants