-
-
Notifications
You must be signed in to change notification settings - Fork 140
feat: http loaded es module realms + HMR DX enrichments #1883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…l-esm-consumption
Cases where the same java classname would result causing a name collision.
…pha testing largely
…l-esm-consumption
…l-esm-consumption
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request refactors the ES module test system and adds HTTP-based ESM loading support with HMR (Hot Module Replacement) capabilities. The changes improve test result parsing, add support for loading ES modules from HTTP(S) URLs, and implement import.meta.hot for development-time module reloading.
Key changes:
- Rewrote ES module tests from CommonJS to native ESM format (.mjs)
- Enhanced test result checker with improved parsing and failure diagnostics
- Added HTTP(S) ESM loader with compile diagnostics and relative import resolution
- Implemented HMR support with import.meta.hot API
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test-app/tools/check_console_test_results.js | Refactored test result parsing to extract structured ES module test summaries, removed tail command from logcat pipeline, improved test status tracking |
| test-app/runtime/src/main/cpp/ModuleInternalCallbacks.cpp | Added HTTP ESM loading, relative URL resolution, HMR support, and diagnostic logging for compile errors |
| test-app/runtime/src/main/cpp/HMRSupport.h | New header defining import.meta.hot API and HTTP fetch utilities |
| test-app/runtime/src/main/cpp/HMRSupport.cpp | Implements HMR callback registration, HTTP fetching via JNI, and URL canonicalization |
| test-app/runtime/src/main/cpp/DevFlags.h | New header for cached script loading log flag |
| test-app/runtime/src/main/cpp/DevFlags.cpp | Implements cached flag for script loading diagnostics |
| test-app/runtime/CMakeLists.txt | Added HMRSupport.cpp and DevFlags.cpp to build |
| test-app/app/src/main/assets/app/tests/testRuntimeImplementedAPIs.js | Improved __time test with better tolerance handling |
| test-app/app/src/main/assets/app/tests/testESModules.mjs | Rewrote tests as async ESM with improved error reporting |
| test-app/app/src/main/assets/app/tests/testESModules.js | Removed old CommonJS test file |
| test-app/app/src/main/assets/app/mainpage.js | Updated to require .mjs test file instead of .js |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while (true) { | ||
| jint n = env.CallIntMethod(inStream, readMethod, buffer); | ||
| if (n < 0) break; // -1 indicates EOF | ||
| if (n == 0) { | ||
| // Defensive: continue reading if zero bytes returned | ||
| continue; | ||
| } |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates a potential infinite loop if the read method consistently returns 0. The loop should have a retry limit or timeout to prevent hanging indefinitely. Consider adding a counter and breaking after a maximum number of consecutive zero-byte reads.
| while (true) { | |
| jint n = env.CallIntMethod(inStream, readMethod, buffer); | |
| if (n < 0) break; // -1 indicates EOF | |
| if (n == 0) { | |
| // Defensive: continue reading if zero bytes returned | |
| continue; | |
| } | |
| int zeroReadCount = 0; | |
| const int MAX_ZERO_READS = 100; | |
| while (true) { | |
| jint n = env.CallIntMethod(inStream, readMethod, buffer); | |
| if (n < 0) break; // -1 indicates EOF | |
| if (n == 0) { | |
| zeroReadCount++; | |
| if (zeroReadCount >= MAX_ZERO_READS) { | |
| // Too many consecutive zero-byte reads, treat as error | |
| break; | |
| } | |
| continue; | |
| } | |
| zeroReadCount = 0; |
|
|
||
| // Get only recent logcat entries and filter for JS entries to reduce output size | ||
| // Also include "{N} Runtime Tests" tag for Jasmine output | ||
| // Use a larger window to capture all failure details |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tail -500 command was removed from the logcat pipeline. This may cause issues with very large logcat outputs that exceed the maxBuffer limit (4MB), potentially missing test results. Consider documenting this change or adding a comment explaining why the tail limit was removed.
| // Use a larger window to capture all failure details | |
| // Use a larger window to capture all failure details | |
| // NOTE: The `tail -500` command was removed from the logcat pipeline to ensure all relevant test results are captured, | |
| // even if there are more than 500 lines. However, this increases the risk of exceeding the maxBuffer limit (4MB), | |
| // which may cause missing test results if logcat output is very large. Consider adjusting the buffer or filtering | |
| // further if this becomes an issue. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request introduces several improvements and refactors related to ES module testing, runtime diagnostics, and internal development flags.
Runtime Diagnostics and Development Flags
DevFlags.cppandDevFlags.hto implement a fast, cached runtime flag for enabling/disabling script loading diagnostics, integrating with the Java runtime for configuration. [1] [2]CMakeLists.txt.