fix(core): require explicit mock adapter construction - #266
Conversation
|
Stacked consumer preview: #268 at exact head The preview remains explicitly draft/non-mergeable and will be rebased to Android-only commits after this prerequisite lands. |
e97e490 to
6baaad4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
6baaad4 to
e65bec3
Compare
kolkov
left a comment
There was a problem hiding this comment.
Eliminating the implicit mock fabrication is the right call — matches Rust wgpu (no mock/noop fallback in Instance::new) and makes registration failures observable rather than silently masked.
Code is correct, tests are thorough, CI clean on all platforms.
Should fix before merge
hal/allbackends/doc.go is stale after the second commit.
The first commit deleted register.go and updated doc.go to say "The software and no-op providers are not registered by this package." The second commit restored register.go (re-adding _ "github.com/gogpu/wgpu/hal/software") but did not update doc.go. Currently the doc claims software is not registered, but register.go imports it.
Suggested: restore the software backend to the registered list in doc.go and keep only noop as "not registered by this package."
Minor (follow-up OK)
After this PR, useMock is true only via NewInstanceWithMock, which never populates deferredGLES. So the mock-removal block in enumerateDeferredGLES (lines 407-409), hasRealAdaptersLocked, and removeMockAdaptersLocked become unreachable dead code. Not blocking — worth a cleanup pass later.
The new TestNewInstanceDoesNotFabricateAdapterWithoutProvider and TestNewInstanceUsesRegisteredProviderWithoutEnablingMock tests cover the key invariants well. LGTM after the doc fix.
|
@kolkov Addressed both the requested documentation correction and the optional cleanup in
The branch is mergeable and all 10 repository Actions checks are green. Could you please take another look when convenient? |
kolkov
left a comment
There was a problem hiding this comment.
Both items addressed in 3e6c836:
doc.go— software correctly listed as registered, noop as explicit-only- Dead code removed —
hasRealAdaptersLocked,removeMockAdaptersLocked, deferred-GLES mock block (-41 LOC)
CI green.
Summary
Remove only the implicit fabricated mock fallback:
core.NewInstancenow remains empty when no registered provider yields an adapterRequestAdapterreports that failure instead of returning a fabricated mock adapterNewInstanceWithMockhal/allbackendskeeps its current desktop software-backend registration behaviorThe public constructors and explicit fallback providers remain available.
Why
A configured provider set that yields no adapter could previously look like successful GPU initialization because core fabricated a test adapter. That masks loader, registration, and platform-support failures.
Failing closed makes provider availability observable while preserving explicit software and mock use.
Validation
Go 1.25.12:
go test ./core ./hal/allbackends .Go 1.26.5:
go test -race ./core .go vet ./core ./hal/allbackends .Focused tests cover empty-provider failure and explicit mock construction.
This correction is independent of the Android backend preview.
Android integration relationship
This Android-independent change is one of five prerequisites replayed in #268. Its canonical candidate head is
e65bec3c74c56cd07265c055ce6a7ec7fbedc680; the corresponding replay in #268 ends ata093f8e969f99f55dfcffdb4b8bcba61cc13299a. The #268 description records the current exact integration head and full replay map; it drops this replay after this PR merges. Rust-v29 typed surface parity remains separate in #273. Android runtime and release claims remain in #268.