Fix emulator race condition in tests #4160
Open
+108
−12
Conversation
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
## Problem Tests occasionally fail with: ``` Uncaught SyntaxError: Got [ERASED] expected EOF ``` Reported in espruino#4132 by @thyttan: tests fail initially but succeed on re-run. Example: https://github.com/espruino/BangleApps/actions/runs/21783161416/attempts/1 ## Cause The test runner sends commands without waiting for the emulator to finish processing. When one test ends and the next begins, `factoryReset()` can be called while the previous test's commands are still being processed, corrupting the input stream. ## Solution - Track emulator ready state (has it output a response?) - Add `waitForResponse()` to poll until command completes - Add `safeFactoryReset()` that ensures previous commands finished - Wait after each test step (`cmd`, `emit`, `load`, etc.) - Wait after app upload and reset in test setup ## Testing - All functional tests pass locally and in CI - Full test suite (all 4 apps): https://github.com/elijahr/BangleApps/actions/runs/21789313739 - No significant increase in test duration
When bin/runapptests.js, .github/workflows/nodejs.yml, or the core submodule changes, run all functional tests instead of only tests for changed apps. This ensures test infrastructure changes are properly validated against the full test suite.
4 tasks
bobrippling
reviewed
Feb 8, 2026
Collaborator
bobrippling
left a comment
bobrippling
left a comment
There was a problem hiding this comment.
Thanks for the rapid fix, this is great - I have some small questions, but nothing major about the design etc :)
| exit 0 | ||
| else | ||
| # Push to other branches: test only changed apps since master | ||
| CHANGED=$(git diff --name-only origin/master...HEAD) |
Collaborator
There was a problem hiding this comment.
Let's keep this comment in, as it's not clear what case the else handles
Suggested change
| CHANGED=$(git diff --name-only origin/master...HEAD) | |
| # Push to other branches: test only changed apps since master | |
| CHANGED=$(git diff --name-only origin/master...HEAD) |
| # Check if test infrastructure changed - if so, run all tests | ||
| RUN_ALL=false | ||
| for f in $TEST_INFRA_FILES; do | ||
| if echo "$CHANGED" | grep -q "^$f$"; then |
Collaborator
There was a problem hiding this comment.
It's minor, but $f is treated as a regex when it isn't. We could:
Suggested change
| if echo "$CHANGED" | grep -q "^$f$"; then | |
| if echo "$CHANGED" | grep -qx "$f"; then |
Or we pull out the refspec above and one-shot this without needing to loop:
# e.g. refspec is origin/master...HEAD
if git diff --quiet "$refspec" -- $TEST_INFRA_FILES; then
...| fi | ||
| done | ||
|
|
||
| if [ "$RUN_ALL" = "true" ]; then |
Collaborator
There was a problem hiding this comment.
We can shorten this:
Suggested change
| if [ "$RUN_ALL" = "true" ]; then | |
| if "$RUN_ALL"; then |
| waitForResponse(); | ||
| } | ||
| if (!emuReady) { | ||
| throw new Error( |
Collaborator
There was a problem hiding this comment.
I suppose this is unreachable code as we can't leave waitForResponse() with emuReady === false
Perhaps we need a way of detecting if maxIterations has been reached?
Comment on lines
248
to
+249
| emu.tx(wrappingCode); | ||
| waitForResponse(); |
Collaborator
There was a problem hiding this comment.
Would using txAndWait() for this and other emu.tx(...); waitForResponse() be an approach to avoiding the bug sneaking in, in future?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Problem
Tests occasionally fail with:
Reported in #4132 by @thyttan: tests fail initially but succeed on re-run.
Example: https://github.com/espruino/BangleApps/actions/runs/21783161416/attempts/1
Cause (suspected)
The test runner sends commands without waiting for the emulator to finish processing. When one test ends and the next begins,
factoryReset()can be called while the previous test's commands are still being processed, corrupting the input stream.Solution
waitForResponse()to poll until command completessafeFactoryReset()that ensures previous commands finishedcmd,emit,load, etc.)Testing
bin/runapptests.js.github/workflows/nodejs.ymlandcoresubmodule).