Skip to content

Conversation

@NathanWalker
Copy link
Contributor

This pull request introduces several improvements and refactors related to ES module testing, runtime diagnostics, and internal development flags.

Runtime Diagnostics and Development Flags

  • Added new C++ files DevFlags.cpp and DevFlags.h to implement a fast, cached runtime flag for enabling/disabling script loading diagnostics, integrating with the Java runtime for configuration. [1] [2]
  • Registered the new diagnostic and HMR support files in the native build by updating CMakeLists.txt.

edusperoni and others added 26 commits August 18, 2025 23:26
Cases where the same java classname would result causing a name collision.
@NathanWalker NathanWalker requested a review from Copilot November 6, 2025 07:20
Copy link

Copilot AI left a 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.

Comment on lines +270 to +276
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;
}
Copy link

Copilot AI Nov 6, 2025

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.

Suggested change
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;

Copilot uses AI. Check for mistakes.

// 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
Copy link

Copilot AI Nov 6, 2025

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.

Suggested change
// 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.

Copilot uses AI. Check for mistakes.
NathanWalker and others added 2 commits November 6, 2025 13:37
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants