Skip to content

Conversation

@jeromelaban
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings January 13, 2026 16:38
Copy link
Contributor

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 PR adds comprehensive unit testing for the WebAssembly variant of the unoicu library. It introduces a validation framework that tests the compiled unoicu.a artifacts using headless Chromium via Puppeteer to ensure the ICU library functions correctly in a browser environment.

Changes:

  • Added validation test harness (C program and JavaScript test runner) to validate unoicu.a artifacts
  • Expanded CI build matrix to include multithreaded and SIMD variants (4 combinations: mt/st × simd/nosimd)
  • Enhanced Dockerfile with cross-compilation support and build configuration for threading/SIMD
  • Added new validate_unoicu job in CI pipeline to test artifacts before packaging

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/unoicu/validation/run-test.js New Puppeteer-based test runner that launches headless Chrome to validate the WASM module
src/unoicu/validation/main.c New C test harness that exercises ICU string conversion functions and reports results to JavaScript
src/unoicu/validation/README.md Documentation explaining the validation harness and its purpose
src/unoicu/Dockerfile Enhanced with cross-compilation support, pkgdata patch, multithreading/SIMD build arguments, and typo fix ("currect" → "current")
.github/workflows/main.yml Expanded build matrix, added validate_unoicu job, updated artifact handling to support multiple configurations, and added package job dependency on validation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +282 to 292
shopt -s nullglob
for dir in unoicu-*; do
new_name="${dir#unoicu-}"
mkdir -p ../nuget/uno.icu-wasm/buildTransitive/native/unoicu.a/"$new_name"/st
cp -r "$dir"/unoicu.a ../nuget/uno.icu-wasm/buildTransitive/native/unoicu.a/"$new_name"/st/unoicu.a
SRC_BASE="$dir/unoicu.a"
if [ ! -d "$SRC_BASE" ]; then
SRC_BASE="$dir"
fi
find "$SRC_BASE" -mindepth 1 -maxdepth 1 -type d ! -name '*nosimd*' -print0 | while IFS= read -r -d '' version_dir; do
cp -R "$version_dir" ../nuget/uno.icu-wasm/buildTransitive/native/unoicu.a/
done
done
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The artifact handling logic is complex with conditional path handling and nosimd filtering in two places (line 289 and 293). Consider documenting why nosimd variants are being excluded from the package, or extracting this logic into a more explicit and maintainable structure.

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +34
SIMD_SUFFIX=$([ "${{ matrix.simd }}" = 'true' ] && echo ',simd' || echo '')
TEMP_SIMD_TAG=$([ "${{ matrix.simd }}" = 'true' ] && echo 'simd' || echo 'nosimd')
TEMP_OUTPUT="out/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}-${TEMP_SIMD_TAG}"
mkdir -p "${TEMP_OUTPUT}"
docker build --build-arg EMSCRIPTEN_VERSION=${{ matrix.EMSCRIPTEN_VERSION }} --build-arg IS_MULTITHREADED=${{ matrix.multithreaded }} --build-arg IS_SIMD_SUPPORTED=${{ matrix.simd }} --output type=local,dest=${TEMP_OUTPUT} .
FINAL_DIR="out/unoicu.a/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}${SIMD_SUFFIX}"
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name 'SIMD_SUFFIX' is misleading as it contains a comma prefix ',simd' (line 27) rather than a true suffix. Consider renaming to 'SIMD_PATH_COMPONENT' or 'SIMD_VARIANT' to better reflect its use in path construction.

Suggested change
SIMD_SUFFIX=$([ "${{ matrix.simd }}" = 'true' ] && echo ',simd' || echo '')
TEMP_SIMD_TAG=$([ "${{ matrix.simd }}" = 'true' ] && echo 'simd' || echo 'nosimd')
TEMP_OUTPUT="out/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}-${TEMP_SIMD_TAG}"
mkdir -p "${TEMP_OUTPUT}"
docker build --build-arg EMSCRIPTEN_VERSION=${{ matrix.EMSCRIPTEN_VERSION }} --build-arg IS_MULTITHREADED=${{ matrix.multithreaded }} --build-arg IS_SIMD_SUPPORTED=${{ matrix.simd }} --output type=local,dest=${TEMP_OUTPUT} .
FINAL_DIR="out/unoicu.a/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}${SIMD_SUFFIX}"
SIMD_PATH_COMPONENT=$([ "${{ matrix.simd }}" = 'true' ] && echo ',simd' || echo '')
TEMP_SIMD_TAG=$([ "${{ matrix.simd }}" = 'true' ] && echo 'simd' || echo 'nosimd')
TEMP_OUTPUT="out/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}-${TEMP_SIMD_TAG}"
mkdir -p "${TEMP_OUTPUT}"
docker build --build-arg EMSCRIPTEN_VERSION=${{ matrix.EMSCRIPTEN_VERSION }} --build-arg IS_MULTITHREADED=${{ matrix.multithreaded }} --build-arg IS_SIMD_SUPPORTED=${{ matrix.simd }} --output type=local,dest=${TEMP_OUTPUT} .
FINAL_DIR="out/unoicu.a/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}${SIMD_PATH_COMPONENT}"

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +34
SIMD_SUFFIX=$([ "${{ matrix.simd }}" = 'true' ] && echo ',simd' || echo '')
TEMP_SIMD_TAG=$([ "${{ matrix.simd }}" = 'true' ] && echo 'simd' || echo 'nosimd')
TEMP_OUTPUT="out/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}-${TEMP_SIMD_TAG}"
mkdir -p "${TEMP_OUTPUT}"
docker build --build-arg EMSCRIPTEN_VERSION=${{ matrix.EMSCRIPTEN_VERSION }} --build-arg IS_MULTITHREADED=${{ matrix.multithreaded }} --build-arg IS_SIMD_SUPPORTED=${{ matrix.simd }} --output type=local,dest=${TEMP_OUTPUT} .
FINAL_DIR="out/unoicu.a/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}${SIMD_SUFFIX}"
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name 'SIMD_SUFFIX' is misleading as it contains a comma prefix ',simd' (line 27) rather than a true suffix. Consider renaming to 'SIMD_PATH_COMPONENT' or 'SIMD_VARIANT' to better reflect its use in path construction.

Suggested change
SIMD_SUFFIX=$([ "${{ matrix.simd }}" = 'true' ] && echo ',simd' || echo '')
TEMP_SIMD_TAG=$([ "${{ matrix.simd }}" = 'true' ] && echo 'simd' || echo 'nosimd')
TEMP_OUTPUT="out/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}-${TEMP_SIMD_TAG}"
mkdir -p "${TEMP_OUTPUT}"
docker build --build-arg EMSCRIPTEN_VERSION=${{ matrix.EMSCRIPTEN_VERSION }} --build-arg IS_MULTITHREADED=${{ matrix.multithreaded }} --build-arg IS_SIMD_SUPPORTED=${{ matrix.simd }} --output type=local,dest=${TEMP_OUTPUT} .
FINAL_DIR="out/unoicu.a/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}${SIMD_SUFFIX}"
SIMD_PATH_COMPONENT=$([ "${{ matrix.simd }}" = 'true' ] && echo ',simd' || echo '')
TEMP_SIMD_TAG=$([ "${{ matrix.simd }}" = 'true' ] && echo 'simd' || echo 'nosimd')
TEMP_OUTPUT="out/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}-${TEMP_SIMD_TAG}"
mkdir -p "${TEMP_OUTPUT}"
docker build --build-arg EMSCRIPTEN_VERSION=${{ matrix.EMSCRIPTEN_VERSION }} --build-arg IS_MULTITHREADED=${{ matrix.multithreaded }} --build-arg IS_SIMD_SUPPORTED=${{ matrix.simd }} --output type=local,dest=${TEMP_OUTPUT} .
FINAL_DIR="out/unoicu.a/${{ matrix.EMSCRIPTEN_VERSION }}/${FEATURE_DIR}${SIMD_PATH_COMPONENT}"

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings January 14, 2026 15:10
Copy link
Contributor

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +100 to +107
docker run --rm -v "$PWD":/src -w /src/validation/app emscripten/emsdk:${{ matrix.EMSCRIPTEN_VERSION }} \
emcc main.c ./unoicu.a \
-I"/src/${ICU_SOURCE_SUBPATH}/common" \
-I"/src/${ICU_SOURCE_SUBPATH}/i18n" \
-sALLOW_MEMORY_GROWTH=1 \
-sEXIT_RUNTIME=1 \
-sASSERTIONS=1 \
-o unoicu_test.html
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation wasm compilation step does not use the same compilation flags (pthread and msimd128) that were used to build the unoicu.a archive being tested. When the archive is built with IS_MULTITHREADED=true, it includes -pthread flag, and when IS_SIMD_SUPPORTED=true, it includes -msimd128 flag. The validation test should use matching flags when compiling against the archive to ensure proper linking and functionality. Add conditional flags based on the matrix parameters to the emcc command.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +17


Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra blank line here. Consider removing one of the blank lines to maintain consistent spacing throughout the file.

Copilot uses AI. Check for mistakes.
@ebariche ebariche self-requested a review January 16, 2026 08:05
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