Repository navigation
Test/web camera release review feedback - #1
Open
snodnipper wants to merge 3 commits into
Open
snodnipper wants to merge 3 commits into
snodnipper wants to merge 3 commits into
Conversation
- Move the duplicated MediaStream helpers and start options into test/web/utils/media_stream_test_utils.dart, shared by both web tests. - Stub getUserMedia and BarcodeDetector through extension type bindings instead of eval(). - Assert the disposed controller does not hold the camera session, via a @VisibleForTesting platformSessionOwner getter, and drop the unused stopCalls counter. - Add reasons to the stop() teardown expectations, and assert that a second PollingBarcodeReader.stop() completes without throwing. - allLive/allEnded no longer pass vacuously on a stream without tracks.
awaitCall() busy-waited, so a start that never reached getUserMedia hung until the runner's 30s timeout with no message. It now waits on a completer and fails after 5s with a TestFailure naming the missing call.
start() awaits the reader's library load (a real script load for zxing-wasm) before calling getUserMedia, with no teardown check between. A scanner dismissed in that window still prompted for permission and briefly lit the camera before the post-acquire check released it. Check the teardown generation immediately before acquiring the camera, and cover it with a test that fails when getUserMedia is reached.
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.
Follow-up to juliansteenbakker#1779, addressing @navaronbracke's nine review comments on the tests, plus one gap found in the start path while doing so.
Review feedback
MediaStreamhelpers and start options move totest/web/utils/media_stream_test_utils.dart, shared by both web tests.getUserMediaandBarcodeDetectorare stubbed through extension type bindings instead ofeval. Restoring usesdelete, which exposes the nativeMediaDevices.prototype.getUserMediaagain, or leavesBarcodeDetectorabsent on browsers that never had it (soisSupported()feature detection stays honest).createJSInteropWrapperdoesn't fit here: it returns a plain object, never a constructor, so it cannot stand in forBarcodeDetector, andnavigator.mediaDevicesis a getter-only accessor, so a wrapper can't be assigned there. I verified thatnewon a.toJSfunction raises the Dart error under both dart2js and dart2wasm.@visibleForTestingplatformSessionOwnergetter next to the existingresetPlatformSessionOwner(). The behavioural check (the next controller's dispose reaches the platform) is kept. The unusedstopCallscounter is gone.stop()completes.allLive/allEndednow fail on a stream with no tracks instead of passing vacuously.awaitCall()no longer busy-waits. IfgetUserMediais never called, it fails after 5s with aTestFailureinstead of hanging until the runner's 30s timeout.Start-path gap
start()awaits the reader's library load (a real script load for zxing-wasm) before callinggetUserMedia, with no teardown check in between. A scanner stopped in that window still prompted for permission and briefly lit the camera before the post-acquire check released it.start()now checks the teardown generation immediately before acquiring the camera. A new test fails without the fix (getUserMediais called once) and passes with it.Verified
flutter test: 386 passed.test/web/polling_barcode_reader_stop_test.dartandtest/web/mobile_scanner_web_start_teardown_test.dartpass in Chrome under dart2js and--wasm.develop'slib/back in fails every new test.test/web/web_library_versions_test.dartfails under--platform chromeondeveloptoo (it readspackage.jsonfrom disk). That failure is unrelated.Related: juliansteenbakker#1778
Checklist
feat: ...,fix: ...,chore: ...) — this is required by CI and drives the automated changelog/release.README.md) was updated, if applicable.