Skip to content

fix: read wRTC dashboard fixtures as UTF-8#4876

Open
Asti1982 wants to merge 1 commit into
Scottcjn:mainfrom
Asti1982:codex/fix-wrtc-dashboard-test-utf8
Open

fix: read wRTC dashboard fixtures as UTF-8#4876
Asti1982 wants to merge 1 commit into
Scottcjn:mainfrom
Asti1982:codex/fix-wrtc-dashboard-test-utf8

Conversation

@Asti1982
Copy link
Copy Markdown

Summary

  • read the wRTC bridge dashboard HTML/JS fixtures with explicit UTF-8 encoding
  • keep the static dashboard tests portable on Windows hosts where the default codec is cp1252
  • add the SPDX header required by the repository checks for the changed test file

Root cause

test_bridge_dashboard.py opened UTF-8 files without an encoding. On Windows, Python can default to cp1252, so the dashboard's Unicode title/arrows raise UnicodeDecodeError before the assertions run.

Validation

  • python -m pytest tools\wrtc-bridge-dashboard\test_bridge_dashboard.py -q -> 27 passed
  • python -m py_compile tools\wrtc-bridge-dashboard\test_bridge_dashboard.py
  • git diff --check -- tools\wrtc-bridge-dashboard\test_bridge_dashboard.py
  • python tools\bcos_spdx_check.py --base-ref origin/main

@github-actions
Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Your PR has a BCOS-L1 or BCOS-L2 label
  • New code files include an SPDX license header
  • You've tested your changes against the live node

Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150)

A maintainer will review your PR soon. Thanks for contributing!

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) size/XS PR: 1-10 lines labels May 12, 2026
Copy link
Copy Markdown

@idan57570-art idan57570-art left a comment

Choose a reason for hiding this comment

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

Code looks clean. Approved for merge.

Copy link
Copy Markdown

@saim256 saim256 left a comment

Choose a reason for hiding this comment

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

Reviewed current head b1af11a. The change is narrowly scoped to the dashboard test fixture reads: all three reads of UTF-8 dashboard files now pass encoding="utf-8", and the touched test file has the required SPDX header. This addresses the Windows default-codepage failure without changing dashboard runtime behavior.

Validation run locally on Windows:

  • python -m pytest tools\wrtc-bridge-dashboard\test_bridge_dashboard.py -q -> 27 passed
  • python -m py_compile tools\wrtc-bridge-dashboard\test_bridge_dashboard.py -> passed
  • git diff --check origin/main...pr-4876 -> passed
  • python tools\bcos_spdx_check.py --base-ref origin/main -> OK

Copy link
Copy Markdown
Contributor

@508704820 508704820 left a comment

Choose a reason for hiding this comment

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

Code Review: Read wRTC Dashboard Fixtures as UTF-8

Summary

Adds explicit encoding="utf-8" to open() calls in the wRTC bridge dashboard tests. Fixes potential UnicodeDecodeError on Windows systems that default to cp1252.

What Works Well

  1. Explicit encoding: Prevents platform-dependent encoding issues
  2. SPDX header: Added MIT license identifier
  3. Low risk: Test-only change

Verdict: Approve

Good encoding practice. Python 3 best practice for cross-platform compatibility.

Copy link
Copy Markdown

@loganoe loganoe left a comment

Choose a reason for hiding this comment

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

Approved. The fixture reads now explicitly use UTF-8, which matches the dashboard files and avoids platform-default encoding failures without changing test behavior.

Validation performed:

  • git diff --check origin/main...HEAD
  • python3 -m py_compile tools/wrtc-bridge-dashboard/test_bridge_dashboard.py
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 pytest -q tools/wrtc-bridge-dashboard/test_bridge_dashboard.py -> 27 passed

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Great contribution! LGTM.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Great contribution! LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) size/XS PR: 1-10 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants