test(extism): cover Eval loadInputData and ConvertToExtismFormat errors#119
Merged
Conversation
Closes #109 Issue #109 listed 6 hard-to-reach error branches in `engines/extism/evaluator/evaluator.go`. Auditing the existing test suite found that 4 of the 6 were already covered: - "plugin instance error" → "error creating plugin instance" (line 443) - "call with context cancellation" → "context cancellation" (line 366) + execHelper "context cancellation" (line 553) - "call with context plain error" → execHelper "execution error" (line 540) - "non-JSON output → string" → "successful execution with string output" (line 185) The remaining two branches in `Eval()` itself were genuinely uncovered: - L172-175: `loadInputData` returning an error from the data provider - L178-181: `internal.ConvertToExtismFormat` failing on json.Marshal Add two new subtests under `TestEvaluator_Evaluate/error_cases`: - "load input data error" — wires `mockErrProvider` (already in this file) into `ExecutableUnit.DataProvider`; verifies Eval returns the wrapped "failed to get input data" error and never touches `plugin.Instance`. - "convert to extism format error" — adds a tiny `mockMapProvider` helper next to `mockErrProvider`; populates it with `chan int` (not JSON-marshalable); verifies Eval returns the wrapped "failed to marshal input data" error and never touches `plugin.Instance`. The `mockMapProvider` is the only new scaffolding — `MockCompiledPlugin`, `mockPluginInstance`, `mockErrProvider`, and `createMockExecutable()` all existed. Coverage: extism/evaluator package moved from ~93% to 95.5%. The two remaining uncovered branches are `instance.Close(ctx)` failure paths (L96-98 in compiler, L118-122 in evaluator) which the issue notes are "logged-only, doesn't propagate, so coverage value is low" — left intentionally untested.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Contributor
There was a problem hiding this comment.
Pull request overview
Improves unit-test coverage for the Extism evaluator by exercising two previously uncovered error branches in Evaluator.Eval() related to input data loading and input marshaling.
Changes:
- Add
mockMapProvidertest helper to return controlledmap[string]anyinput without errors. - Add subtest covering
loadInputDatafailure (failed to get input data) and assertingplugin.Instanceis not reached. - Add subtest covering
internal.ConvertToExtismFormatmarshaling failure (failed to marshal input data) and assertingplugin.Instanceis not reached.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
robbyt
added a commit
that referenced
this pull request
May 10, 2026
* docs: add CHANGELOG.md (Keep a Changelog 1.1.0) Closes #102 Repo had no CHANGELOG.md; users had to read GitHub release notes to find what changed between versions. With v1 ahead and a queue of breaking changes (#86, #87, #88, #89, #90, #91, #104), having a CHANGELOG before landing those gives downstream code reviewers and IDEs visibility into what's coming. Format follows Keep a Changelog 1.1.0 with the standard six headings (Added / Changed / Deprecated / Removed / Fixed / Security). [Unreleased] section captures the run of merged-but-not-released PRs since v0.7.0: - #103 polyscript.New[E] generic constructor (deprecates 12 FromXxx) - #105 slog.Handler optional in engine subpackages - #106 RequestToMap mutation fix - #107 HTTP loader MaxBodySize cap - #110 unify nil-handler on slog.Default(); drop stdout fallbacks - #113 WithLogHandler(nil)/WithLogger(nil) → no-op - #114 drop redundant nil-guards; tighten WithLogHandler doc - #115 docs/LOGGING.md - #117 fix bare top-level json.Number leak - #118 WithGlobals additive; drop dead URL check - #119 extism Eval test coverage Backfilled the three releases the issue called out (v0.5.0, v0.6.0, v0.7.0) from existing GitHub release notes, sorted into Keep a Changelog categories. Earlier releases (v0.0.x through v0.4.0) intentionally not backfilled per the issue scope; can be added in a follow-up if desired. Out of scope: the optional CI gate that fails PRs not touching CHANGELOG.md. Adds noise to small bug PRs and wants opt-out-label infrastructure to support it; better as a separate issue. * docs(CHANGELOG): clarify deprecated FromXxx still take positional handler Copilot review noted that "no constructor demands a handler" is misleading because the deprecated FromXxx constructors still take a positional `logHandler slog.Handler` argument (even though nil is accepted). Reword to distinguish the new generic constructor (no handler arg) from the deprecated ones (still take it, but nil is OK). --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #109 listed 6 hard-to-reach error branches in
engines/extism/evaluator/evaluator.go. Auditing the existing test suite found that 4 of the 6 were already covered:plugin instance errorTestEvaluator_Evaluate/error_cases/error creating plugin instance(L443)call with context cancellationTestEvaluator_Evaluate/error_cases/context cancellation(L366) +metadata_tests/exec helper/context cancellation(L553)call with context plain errormetadata_tests/exec helper/execution error(L540)non-JSON output falls back to stringsuccess_cases/successful execution with string output(L185) — passes plain text bytes, asserts string fallbackThe remaining two branches in
Eval()itself were genuinely uncovered. This PR adds two subtests underTestEvaluator_Evaluate/error_cases:1.
load input data error— coversevaluator.go:172-175Wires the existing
mockErrProviderintoExecutableUnit.DataProvider. VerifiesEvalreturns the wrapped"failed to get input data"error and never reachesplugin.Instance.2.
convert to extism format error— coversevaluator.go:178-181Adds a tiny
mockMapProviderhelper next tomockErrProvider; populates it withchan int(not JSON-marshalable). VerifiesEvalreturns the wrapped"failed to marshal input data"error and never reachesplugin.Instance.Scaffolding
mockMapProvideris the only new scaffolding —MockCompiledPlugin,mockPluginInstance,mockErrProvider, andcreateMockExecutable()all existed.Out of scope
The
instance.Close(ctx)failure paths (L96-98 in compiler, L118-122 in evaluator) — the issue body notes these are "logged-only, doesn't propagate, so coverage value is low." Left intentionally untested.The exit-code-vs-output design question (#94) — separate issue.
Test plan
go test -race -count=1 ./engines/extism/evaluator/...— green; 2 new subtests visiblego test -race -count=1 ./...— full suite greengo vet ./...— cleanCloses #109
https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Generated by Claude Code