test: fix cross-platform failures in InputManager & TextUtils#3044
test: fix cross-platform failures in InputManager & TextUtils#3044cptbtptpbcptdtptp wants to merge 3 commits into
Conversation
The "keyboard" case dispatches a `keydown KeyA` without a following `update`, so the event stays queued and is later consumed by another case's `update`, marking KeyA as held down. On mac the meta-key cleanup in KeyboardManager happens to clear it, but on other platforms it stays held, so the next `keydown KeyA` is filtered as a repeat and isKeyDown returns false. This made "blur and focus" and "change listener target" fail locally on Windows while passing in CI (macOS). Reset the keyboard state at the end of the case to keep it isolated. Refs galacean#3043 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TextUtils derives lineHeight from a per-pixel scan of rendered glyphs, so relying on the host "Arial" (a different font file and rasterizer per OS) produced platform-dependent values (lineHeight 27 on macOS vs 26 on Windows). Bundle a small Roboto subset (Apache-2.0), load it via FontFace in the test, point the renderers at it, and rebaseline the metrics. CJK text still falls back to system fonts as before, so its full-width advances stay stable. Refs galacean#3043 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTwo test-only fixes: ChangesTextUtils: Roboto subset font and updated metric assertions
InputManager: keyboard state leak fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3044 +/- ##
===========================================
- Coverage 79.28% 79.28% -0.01%
===========================================
Files 919 919
Lines 102452 102452
Branches 11354 11353 -1
===========================================
- Hits 81230 81225 -5
- Misses 21034 21039 +5
Partials 188 188
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
两处均为测试修复,方向正确。
- InputManager:根因定位准确。
keyboard用例里keydown KeyA之后没有update,事件滞留在_nativeEvents队列,被后续用例的update消费;Mac 上KeyboardManager._update的 meta 键清理分支恰好把它清掉,非 Mac 平台不走该分支,于是KeyA残留在_curHeldDownKeyToIndexMap,下一次keydown KeyA被当作重复过滤掉 → 失败。修复用blur(清空所有按键 map 和_nativeEvents)+update(排空队列),对照KeyboardManager.ts核实无误,是最小且对根因的修复,不是掩盖 flakiness。第 198 行isKeyHeldDown(KeyA) === false的断言在 update 之前读 map,语义不受影响。 - TextUtils:内置一个真实的 Roboto-Regular 子集(name 表核实为 v2.137、Apache-2.0、~22KB),通过
FontFace/document.fonts加载。这让 advance 类指标(width/lines/lineWidths)真正跨平台确定——字体字节在所有平台一致,measureText的步进不依赖光栅器。回归测试反转可破(字体换回 Arial 则 macOS 给 27、值停在 28 即失败;保留字体把值改回 27 则 Roboto 给 28 即失败),约束有效。
无阻塞问题,方向上严格优于现状。
问题
[P2] lineHeight/height 的光栅器依赖仍未根除(TextUtils.test.ts,所有 lineHeight/height 断言,如第 179/277/502/537 行)
换固定字体只消除了"各 OS 的 Arial 是不同字体文件"这一项方差,但没有消除该断言失败的真正根因:lineHeight = fontSizeInfo.size + lineSpacing,而 size = ascent + descent 来自 TextUtils._measureFontOrChar 里的 getImageData 逐像素 alpha 扫描(TextUtils.ts:397/425-428)。这个扫描依赖光栅器(CoreText vs DirectWrite vs FreeType),即便 Roboto 字形轮廓完全一致,顶/底边缘像素仍可能差 ±1px。新基线 28 是 CI(macos-latest)/CoreText 下的值;提 #3043 的 Windows 同学很可能仍拿到 27 而继续红——而且本 PR 只在非 Mac 平台验证了 InputManager,TextUtils 没有在 Windows 上验证过。
建议:对光栅化的纵向指标用容差断言,例如 expect(result.lineHeight).to.be.closeTo(28, 1)(height 是 lineHeight * 行数,同理),同时把 advance 类的 width/lineWidths/lines 保持精确相等。这样既守住回归意义,又容忍作者自己已承认可能存在的 ±1px。
[P3] 内置字体缺少就地的来源/许可说明(Roboto-Regular-subset.ttf)
子集的 name 表只保留了简短版权串("Copyright 2011 Google Inc."),许可条目(Apache-2.0)在子集化时似乎被丢掉了(二进制里 strings 不到 "Apache"/"License")。PR 描述写明了 Apache-2.0 是对的,但仓库里落了个无就地溯源的二进制 blob。建议在测试里加一行注释或在文件旁放一行 NOTICE 记录"Roboto-Regular v2.137, Apache-2.0, subset for tests",便于追溯许可来源。
[P3] 字体可用性可更稳(TextUtils.test.ts:35-36)
await testFont.load() + document.fonts.add(testFont) 之后字体已加载、canvas 一般能拿到;如再 await document.fonts.ready,可在任何测量前彻底消除字体就绪竞态。可选,非必须。
简化建议
无。InputManager 的修复是最小化的局部清理(用 afterEach 统一 blur/update 更系统,但对单个泄漏用例属过度设计);TextUtils 的 new URL(..., import.meta.url) + FontFace 加载是 Vite/browser-mode 的惯用写法,没有引入多余复杂度。
FAIL @galacean/engine-tests (chromium) src/core/2d/text/TextUtils.test.ts > TextUtils > measureTextWithWrap
AssertionError: expected [ 'T', 'h', 'e ', 'w', 'e', …(11) ] to deeply equal [ 'T', 'h', 'e ', 'w', 'e', …(11) ]
Failure screenshot:
- tests/src/core/2d/text/__screenshots__/TextUtils.test.ts/TextUtils-measureTextWithWrap-1.png
- Expected
+ Received
@@ -11,8 +11,8 @@
"gr",
"e",
"at",
"to",
"d",
- "ay",
- ".",
+ "a",
+ "y.",
]
❯ src/core/2d/text/TextUtils.test.ts:190:36
188| expect(result.width).to.be.equal(24);
189| expect(result.height).to.be.equal(100);
190| expect(result.lines).to.be.deep.equal([
| ^
191| "T",
192| "h", |
Address review feedback on galacean#3044: - lineHeight/height derive from a per-pixel glyph scan and stay rasterizer dependent (CoreText vs DirectWrite) even with a fixed font, so assert them with a +/-1px tolerance (closeTo) while keeping advance-based width/lines/lineWidths exact. This preserves the regression guard while letting non-mac platforms pass too. - Record the bundled font source/license inline (Roboto-Regular v2.137, Apache-2.0). - Await document.fonts.ready to remove a font-readiness race. Refs galacean#3043 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
windows端依旧相同的报错,测试不通过 |
Summary
Fixes the local (non-mac) unit-test failures reported in #3043. Both are test issues, not engine bugs — and both were hidden because the test job runs on
macos-latest.InputManager —
blur and focus,change listener targetRoot cause is cross-case state leakage, masked by a mac-only branch. The
keyboardcase dispatches akeydown KeyAwith no followingupdate, so the event stays queued and is consumed by a later case'supdate, markingKeyAas held down. On mac the meta-key cleanup inKeyboardManagerhappens to clear it; on other platforms it stays held, so the nextkeydown KeyAis filtered as a repeat andisKeyDownreturnsfalse.Fix: reset the keyboard state at the end of the
keyboardcase so it doesn't leak.TextUtils —
measureTextWithWrap,measureTextWithoutWraplineHeightis derived from a per-pixel scan of glyphs rendered with the hostArial, which is a different font file and rasterizer per OS (27 on macOS, 26 on Windows).Fix: bundle a small Roboto subset (Apache-2.0, ~22 KB, Latin only), load it via
FontFace, point the renderers at it, and rebaseline the metrics. Advance-based metrics (width/lines/lineWidths) now come from the embedded file and are stable across platforms; CJK text still falls back to system fonts as before, so its full-width advances are unchanged.Verification
Reproduced the InputManager failures locally by forcing a non-mac platform (both target cases then fail exactly as in the issue), and confirmed both fixes make all 14 tests pass.
Closes #3043
Summary by CodeRabbit