feat: framework modernization (phases A-F) - #51
Conversation
Phase A - Foundational fixes: - Remove dead ts-node dependency from e2e-test-app - Replace bytes package with inline formatBytes utility - Fix chai phantom dependencies in web-application and core/types - Add exports field to timeout-config package.json - Fix init command template (vitest -> ringai API) - Convert eslint.config.js to ESM (eslint.config.mjs) Phase B - Playwright migration: - Migrate all deprecated APIs: page.$()/$$() -> locator(), waitForSelector() -> locator().waitFor() - Upgrade Playwright ^1.48 -> ^1.58 across 3 packages - Remove deprecated devtools option from example config Phase C - Core library upgrades: - TypeScript 5.6.3 -> 5.9.3, typescript-eslint 8.56 -> 8.33 - nanoid 3 -> 5, p-limit 3 -> 7, citty ^0.1.6 -> ^0.2.1 - chai 4 -> 5 (remove @types/chai, chai 5 ships own types) - Fix hookable private _hooks access with internal Set tracking Phase D - Reporter system integration: - Create TestResultCollector adapter bridging TestRunController hooks to ReporterManager events - Wire ReporterManager into runCommand.ts - Implement AI JSON reporter (ai-json) with error categorization, flaky detection, environment metadata Phase E - Playwright new features (RPC-compatible): - Add storageState with IndexedDB support - Add emulateMedia with contrast option - Add CDP connectOverCDP with cdpEndpoint/cdpIsLocal config Phase F - Code quality: - Translate all Chinese comments to English across 10 files - Clean up any casts: type BrowserClientItem properly, remove unnecessary (browser as any).isConnected Made-with: Cursor
Update typescript-eslint 8.33.0 -> 8.56.1 to resolve unmet peer dependency with TypeScript 5.9.3 (8.33.0 required <5.9.0). Made-with: Cursor
Greptile SummaryThis PR modernizes the ringai testing framework across six coordinated phases. Phase A removes dead dependencies (ts-node, bytes package) and fixes configuration issues (init command template, ESM eslint config, exports field). Phase B migrates all Playwright APIs from deprecated Key improvements:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[RunCommand.run] --> B[Create TestRunController]
A --> C[Create ReporterManager]
C --> D[Initialize Reporters<br/>ai-json, spec, etc.]
A --> E[Create TestResultCollector]
E --> F[Register Hooks with<br/>TestRunController]
B --> G[Test Execution Starts]
G --> H[beforeRun Hook]
H --> I[ReporterManager.start]
G --> J[Test Loop]
J --> K[beforeTest Hook]
K --> L[Record Start Time]
J --> M[Test Execution]
M --> N[afterTest Hook]
N --> O{Test Result}
O -->|Pass| P[Build TestResult<br/>status: passed]
O -->|Fail| Q[Build TestResult<br/>status: failed<br/>+ error details]
P --> R[ReporterManager.testPass]
Q --> S[ReporterManager.testFail]
R --> T[AI Reporter:<br/>Categorize & Store]
S --> T
J --> U[afterRun Hook]
U --> V[ReporterManager.end]
V --> W[AI Reporter:<br/>Generate JSON Report<br/>with metadata]
W --> X[Output Report<br/>with error categories,<br/>flaky detection,<br/>environment data]
style C fill:#e1f5ff
style E fill:#e1f5ff
style T fill:#fff4e1
style W fill:#fff4e1
style X fill:#d4edda
Last reviewed commit: fbdabfc |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (this.config.cdpEndpoint) { | ||
| const cdpOptions: any = { wsEndpoint: this.config.cdpEndpoint }; | ||
| if (this.config.cdpIsLocal !== undefined) { | ||
| cdpOptions.isLocal = this.config.cdpIsLocal; | ||
| } | ||
| this.browser = await chromium.connectOverCDP(this.config.cdpEndpoint, cdpOptions); |
There was a problem hiding this comment.
wsEndpoint in options may be redundant since endpoint is already passed as first parameter. Consider simplifying:
| if (this.config.cdpEndpoint) { | |
| const cdpOptions: any = { wsEndpoint: this.config.cdpEndpoint }; | |
| if (this.config.cdpIsLocal !== undefined) { | |
| cdpOptions.isLocal = this.config.cdpIsLocal; | |
| } | |
| this.browser = await chromium.connectOverCDP(this.config.cdpEndpoint, cdpOptions); | |
| if (this.config.cdpEndpoint) { | |
| const cdpOptions: any = this.config.cdpIsLocal !== undefined | |
| ? { isLocal: this.config.cdpIsLocal } | |
| : {}; | |
| this.browser = await chromium.connectOverCDP(this.config.cdpEndpoint, cdpOptions); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Update typescript-eslint 8.33.0 -> 8.56.1 to resolve unmet peer dependency with TypeScript 5.9.3 (8.33.0 required <5.9.0). Made-with: Cursor
Update typescript-eslint 8.33.0 -> 8.56.1 to resolve unmet peer dependency with TypeScript 5.9.3 (8.33.0 required <5.9.0). Made-with: Cursor
danbao
left a comment
danbao
left a comment
There was a problem hiding this comment.
基于 main@6014967 到 29f60bc 的完整 diff、现有 Actions 结果及本地构建/测试做了复核。
改动概览
这个 PR 同时引入了 Hookable 注册接口、Reporter 与 CLI 的连接、ai-json Reporter、Playwright 1.58/CDP 与 storage/media API，以及一批依赖和工程配置升级。方向上有价值：Reporter 抽象、显式的浏览器状态 API、timeout 包 exports、移除旧依赖等都值得保留；本地 build:main 的 26 个 package 也全部构建成功，lint 为 0 error。
阻断问题
-
当前 Reporter 接入会把实际测试队列清空，并可能产生“0 个测试成功”的假绿。
TestResultCollector.registerHooks()给beforeRun注册了 Hookable handler；随后PluggableModule.callHook()因registeredHookableNames.has('beforeRun')改走hookable.callHook()。但 Hookable 的callHook()是事件通知语义，不会返回 handler 的testQueue。运行时探针得到的返回值是undefined，而TestRunController.prepareTests()又执行(modifierQueue || []).map(...)，最终队列长度变成 0。workerLimit 为数字时会创建 0 个 worker，然后正常返回成功。这个问题必须先修复，并增加一条从 CLI/Reporter 到 TestRunController 的真实集成测试。 -
同一分流还会绕过所有旧插件的生命周期 Hook。
只要同名的新 Hookable handler 存在，callHook()就完全不执行原有LegacyHook。因此beforeRun/beforeTest/afterTest/beforeTestRetry/afterRun上已有插件都会失效，而不是“向后兼容”。需要明确 modifier/listener 两种语义并组合执行，不能按 hook 名二选一。返回的 disposer 也没有同步清理registeredHookableNames，dispose 后仍不会回退到 legacy。 -
Reporter 汇总总数恒为 0。
Collector 已知道testQueue.length，但调用的是reporterManager.start([])；Manager 又用参数长度初始化state.total。最小探针执行 2 个测试中的 1 个通过后得到{total:0, passed:1}，所以 JSON/AI JSON 的 total 与 successRate 都不可信。 -
PR 自身测试没有通过。
本地结果为81 passed / 1 failed（1183/1184），失败在core/cli/test/runCommand.spec.ts：现有 TestRunController mock 没有新增的registerHook。远端 6 个 Node/OS 矩阵 job 也全部是 failure。这个失败虽然只是 mock 未更新，但恰好说明新增连接没有覆盖兼容契约；不能在红色测试下合并。
安全与可运维性
ai-json 会原样输出 error message、完整 stack、测试文件路径、OS release 和 Node 版本。测试异常经常包含绝对路径、请求内容或 token 片段，而该格式又明确用于提交给 LLM/自动分析工具，会把数据带出原信任边界。建议在启用前提供默认脱敏（路径、URL query/header/常见凭据）、可配置字段白名单，并在文档中明确输出敏感性；不要默认把原始 stack 当作可外发数据。
此外，这个 PR 横跨 Hook 核心语义、CLI、Reporter、浏览器驱动和多项 major/pre-1.0 依赖升级，出现回归时很难定位。建议至少拆成：依赖升级、Hookable 迁移、Reporter 接入、Playwright 新能力四组。
结论
当前不适合合并。 先修复测试队列/legacy hook/Reporter total 三个行为问题，使现有矩阵恢复绿色，并补上“非空队列真实执行 + legacy 与新 hook 共存 + Reporter 正确汇总”的集成测试后再审。
danbao
left a comment
danbao
left a comment
There was a problem hiding this comment.
增量复审（2026-07-13）
复审基准仍是 main@6014967c -> 29f60bc3。GitHub 当前 head 与上一轮 review 绑定的 commit 完全相同，没有新增 commit；Actions 也仍是原来的 6 个 Node/OS CI job 全部失败。
因此上一轮的阻断项都仍然存在：
- Reporter 注册
beforeRun后仍会把 Hookable 的undefined返回值当成修改后的队列，最终执行空队列并可能假绿； - 同名 Hookable handler 仍会绕过 legacy 生命周期 hooks，disposer 也未恢复 fallback；
reporterManager.start([])仍使 total 恒为 0；core/cli/test/runCommand.spec.ts的registerHook契约失败仍未修；ai-json的原始 error/stack/path 外发脱敏边界仍未补齐。
复审结论：仍不适合合并。 当前没有可验证的修复增量；请先推送修复 commit，并让 exact head 的单元测试与 Node 22/24 全平台矩阵恢复绿色后再复审。
1 participant
Phase A - Foundational fixes:
Phase B - Playwright migration:
Phase C - Core library upgrades:
Phase D - Reporter system integration:
Phase E - Playwright new features (RPC-compatible):
Phase F - Code quality:
Made-with: Cursor