fix(ci): stop swallowing web test failures; pin dart:io fixture tests to vm#363
Merged
Conversation
… to vm Closes the remaining two parts of #353. Un-swallow: the `test:web:single` melos script's exit status was that of its LAST statement -- the trailing WITH_WASM if-block, which evaluates to 0 whenever WITH_WASM != true -- so `dart test --platform chrome/firefox` failures were silently discarded and `unit_tests` stayed green while 27 web tests failed on main. `set -e` restores propagation. The same trailing- statement shape existed latently in five other chained scripts (`test`, `test:web`, `coverage:format`, `coverage:combine`, `generate`); all get `set -e`. In `coverage:combine` this is load-bearing: a combine_coverage failure was maskable by remove_from_coverage succeeding on a stale file. Web-correctness: the tests those failures pointed at read PEM/DER fixtures from disk via dart:io, which cannot exist on the web platform ("Unsupported operation: _Namespace" from _File.readAsBytesSync). Pin exactly the fixture-reading groups to `testOn: 'vm'`, keeping every pure-parsing group cross-platform: x509_plus/test/x509_test.dart 6 groups (rsa, ec, csr, rfc5280, keys from auth services, Certificate fields) x509_plus/test/util_extra_test.dart 3 groups x509_plus/test/x509_base_extra_test.dart 2 groups jose_plus/test/jwk_test.dart 1 group (JWK from pem files) The util_extra/x509_base_extra pins also cover the two dart:io-using files added by #355, which would otherwise have joined the hidden-failure list. Verified locally with the un-swallowed semantics, all six runs green: x509_plus chrome 47/47 firefox 47/47 (was 8 pass / 17 fail) jose_plus chrome 212/212 firefox 212/212 (was 98 pass / 6 fail) oidc_web_core chrome 27/27 firefox 27/27 (Firefox needed #360) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughTest groups involving VM-only operations are now restricted to the Dart VM. Melos generation, test, web-test, and coverage scripts now use fail-fast shell behavior. ChangesTest and CI execution
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 @@
## main #363 +/- ##
===========================================
+ Coverage 61.53% 71.66% +10.13%
===========================================
Files 158 158
Lines 8392 8446 +54
Branches 1572 1572
===========================================
+ Hits 5164 6053 +889
+ Misses 3228 2393 -835 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ahmednfwela
added a commit
that referenced
this pull request
Jul 11, 2026
…wser loads fast Run 29118828702's unit_tests jobs died at GitHub's 6-hour limit: on the Firefox step, four packages finished by 21:07 and the remaining two (oidc_web_core, oidc_loopback_listener) never printed a single line for 4.5 hours -- the job-kill cleanup shows a pile of orphaned Firefox helper processes, i.e. a wedged browser launch. The code is exonerated: the full oidc_web_core suite passes 36/36 on local Firefox in 2 seconds, and loopback's suite is vm-pinned. The amplifier was our own flag: --ignore-timeouts disables ALL package:test timeouts including the browser-load timeout, so a routine launch flake became an infinite hang. Keep --timeout 60m (jose_plus's crypto tests are legitimately slow under dart2js) and drop --ignore-timeouts from the three web-test invocations: a wedged load now fails loudly in minutes and the job can simply be rerun. The iOS integration script's --ignore-timeouts is a different context and is untouched. Also adds the set -e that the #363 sweep missed on test:coverage (same trailing-statement swallow shape; not CI-invoked, fixed for consistency). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2
ahmednfwela
added a commit
that referenced
this pull request
Jul 11, 2026
#368) * ci(coverage): unify noise policy — ignore-marker parity and generated-file strips Three gaps in what "covered" means, found while auditing the denominator: * Pure-Dart packages ignored // coverage:ignore-* markers: flutter test honors them natively (ignore-marked platform.dart never appears in Flutter lcov) but coverage:format_coverage only does so with --check-ignore (bin/format_coverage.dart:223, checkIgnoredLines). Added the flag so both halves of the repo obey the same ignore semantics. * oidc_cli/lib/src/version.dart is generated ("Generated code. Do not modify.") but does not match the *.g.dart convention -- added a second remove_from_coverage pattern (multi -r verified empirically against remove_from_coverage 2.0.0). * The upload-coverage job combined integration lcov straight into the Codecov upload with NO generated-file strip at all -- *.g.dart from integration runs reached Codecov. Added the same strip step there, and codecov.yml ignore globs as the reporting-side backstop (generated files, example app, version.dart). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * ci(coverage): harvest full workspace coverage from patrol integration runs The android/ios/linux/windows patrol jobs passed `--coverage-package oidc`. patrol_cli_plus treats that as an UNANCHORED regex over package names (commands/test.dart:222 -> RegExp.hasMatch in coverage_tool.dart:355), so all oidc* packages were already harvested -- but jose_plus, x509_plus and crypto_keys_plus were not, despite the conformance flows exercising them heavily (JWT signature verification, JWKS certificate parsing). The artifact numbers agree: jose_plus integration delta over unit coverage was exactly zero. Switch the four native patrol jobs to `--coverage-workspace`, the fork's purpose-built flag that includes every `workspace:` member from the root pubspec (coverage_tool.dart:358-371). Verified the flag exists in the published patrol_cli_plus 5.4.1 that CI installs: it landed in dee5f944a, which is contained in the patrol_cli_plus-v5.4.1 tag. The macos job uses plain `flutter test`, whose --coverage-package is a regex over package names (flutter test --help); replace the sloppy `"oidc*"` (regex, not glob: matches "oid" + any number of "c") with an anchored `"^(oidc|jose_plus|x509_plus|crypto_keys_plus)"`. The web job keeps `--coverage-package oidc`: its V8->lcov collector is a separate best-effort path (test.dart:402 guards the VM-service coverage route to !isWeb). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * fix(oidc_core): actually clear query/fragment when deriving issuer from well-known URI Closes #356. `Uri.replace(query: null, fragment: null)` keeps the existing components (null means "unchanged"), so getIssuerFromOpenIdConfigWellKnownUri returned issuers carrying the original query/fragment, contradicting its own doc. `query: ''` is no fix either -- it serializes a dangling '?'. Build the issuer fresh from components instead (userInfo omitted when empty, port omitted when !hasPort so no default port is injected). Behavior note: a query-carrying well-known URL with the standard path layout (e.g. Entra's `?appid=` form) now inverts to a clean issuer instead of a query-carrying one; the doc comment previously called that form "cannot be inverted" and has been corrected. Downstream issuer mix-up defense stays with issuersAreIdentical. Tests: both pre-existing happy-path tests pass byte-identical; new tests cover query+fragment stripping (hasQuery/hasFragment false, no dangling '?'), the Entra `?appid=` derivation, and userInfo/non-default-port preservation. Adversarially verified by an independent reviewer lane. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * fix(x509_plus): stop double-encoding OIDs in toAsn1; parse NoticeReference numbers Closes #357, #358. #357: ObjectIdentifier.toAsn1() manually base-128-encoded the OID content bytes and handed them to ASN1ObjectIdentifier(...), whose constructor (asn1lib 1.6.5) expects plain arc components and encodes them itself -- every re-encode path (AlgorithmIdentifier/Name/SubjectPublicKeyInfo/ X509Certificate.toAsn1, toPem) emitted doubly-encoded, corrupt DER. Now `return ASN1ObjectIdentifier(nodes);`. Proven by round-trip tests over five real OIDs, an independent DER cross-check re-parsing toAsn1().encodedBytes with ASN1Parser, and a vm-only test re-encoding rfc5280_cert1.cer's subject/issuer Names and recovering the original OIDs. #358: NoticeReference.fromAsn1 assigned toDart(...)'s List<dynamic> (of BigInt) to List<int> noticeNumbers -- a TypeError making any UserNotice with a noticeRef unparseable. Now mapped explicitly to List<int>. Proven by an in-memory noticeRef parse test ([1,2,300]). Both replace tests that previously had to avoid the buggy paths. Adversarially verified by an independent reviewer lane. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * fix(jose_plus): JoseHeader.critical no longer throws on headers carrying crit Closes #359. `getTyped('crit')` returned the raw List<dynamic> JSON value cast to List<String>?, throwing a TypeError on any header that actually uses the parameter the getter exists for. Use getTypedList('crit'), exactly as `audience` does for 'aud' (lib/src/jwt.dart:37). Tests cover both the crit-present (['exp']) and crit-absent (null) reads. Adversarially verified by an independent reviewer lane. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * test: surface-load every package and drive unit coverage toward 95% Second campaign wave (10-lane parallel workflow, every lane independently adversarially verified in a fresh context). Surface honesty: each of the 16 packages gains test/library_surface_test.dart importing its public barrels with meaningful assertions, so every file with coverable lines enters the coverage denominator -- lcov only ever sees LOADED libraries, so real code could previously hide entirely (and did: oidc_web_core/src/user_manager_web.dart, 94 raw lines, appeared in no report). Files still absent after loading are provably noise: the VM collector emits no source entry for zero-executable-statement files (verified against the raw coverage JSON for the empty-class par/req.dart and loopback mock.dart, and const-only native_channel_constants.dart). Cover lanes (verifier-measured, per own probes): oidc_core 1838/2247 -> 2135/2247 (95.0%) 3 new suites incl. user-manager flow/internals coverage oidc_cli 444/624 -> 539/581 (92.8%) crypto/x509/jose 2320/2367 -> 2360/2367 (99.7%) flutter tails 429/457 -> 474/474 (100%) web pair 216/237 -> 284/421 (oidc_web.dart is now visible and honestly uncovered -- see the structural finding below) Policy-compliant coverage:ignore markers (each with justification, each audited by the lane verifier as genuinely unit-untestable): oidc_cli's real-browser Process.run launches and the `dart pub token add` shell-out (mutates the developer's real pub credential store; no injection seam). Also removes a stale coverage:ignore-file from oidc_platform_interface's platform.dart, whose 17 lines were already fully covered. Known limitation recorded, not hidden: oidc_web/lib/oidc_web.dart compiles only for browser targets, and CI's `flutter test` runs on the VM, so its @teston('chrome') surface test is silently skipped there; its lines stay visible-and-uncovered in the unit artifact until the tooling gap is closed (tracked in a follow-up issue). Dead-code findings in oidc_cli login commands are documented in the coverage tests rather than enshrined. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * test: pin certificate fixture test to vm; make pub-proxy forward hermetic Review findings on #368 (one blocking, two advisory), all three addressed: * certificate_extra_test.dart read DER fixtures in setUpAll with no platform pin -- the one unpinned dart:io fixture reader among the new tests, and a guaranteed failure on the un-swallowed `dart test --platform chrome` CI step. Now @teston('vm'), matching the #353 convention. Verified: x509_plus on chrome 60/60 (previously failed in isolation), vm 99/99. * The one pub-proxy test that reaches the real `dart` binary forwarded `cache list`, whose runtime scales with the machine's global pub cache (timed out at 30s on a large dev cache). Forward `token list` instead: read-only and O(token store). 5/5 in ~8s. * dart_pub.dart's ignore-file rationale leaked an authoring-session phrase ("the hard constraint elsewhere in this task"); trimmed to the technical rationale only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * ci(tests): drop --ignore-timeouts from web test runs; fail wedged browser loads fast Run 29118828702's unit_tests jobs died at GitHub's 6-hour limit: on the Firefox step, four packages finished by 21:07 and the remaining two (oidc_web_core, oidc_loopback_listener) never printed a single line for 4.5 hours -- the job-kill cleanup shows a pile of orphaned Firefox helper processes, i.e. a wedged browser launch. The code is exonerated: the full oidc_web_core suite passes 36/36 on local Firefox in 2 seconds, and loopback's suite is vm-pinned. The amplifier was our own flag: --ignore-timeouts disables ALL package:test timeouts including the browser-load timeout, so a routine launch flake became an infinite hang. Keep --timeout 60m (jose_plus's crypto tests are legitimately slow under dart2js) and drop --ignore-timeouts from the three web-test invocations: a wedged load now fails loudly in minutes and the job can simply be rerun. The iOS integration script's --ignore-timeouts is a different context and is untouched. Also adds the set -e that the #363 sweep missed on test:coverage (same trailing-statement swallow shape; not CI-invoked, fixed for consistency). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * ci(tests): bound web suite loads at 4m — wedged browser launches fail fast The ed7b535 run proved the Firefox launch wedge recurs: its unit_tests jobs sat on the Firefox step for 3h13m (vs ~66min for the entire step on green runs). Dropping --ignore-timeouts made the wedge BOUNDED but not cheap: with no explicit --suite-load-timeout, package:test scales the suite connect/load timeout up to the per-test cap (60m), so every wedged launch still burned up to an hour before failing. --suite-load-timeout 4m (flag verified in dart test --help; load includes the dart2js compile) is ~5x the worst legitimate suite compile+load observed on green CI and converts a wedge into a fast, named failure that a simple job rerun clears. Applied to all three web-test invocations; implements the reviewer's advisory from the ed7b535 approval. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * ci(tests): exclude slow web-crypto suites from PR web runs; drop suite-load bound My --suite-load-timeout 4m in melos test:web:single false-failed 8 suites on the beta chrome step of run 29140552735 ("loading <file> (failed)" + "TimeoutException after 0:04:00"). The timer evidently spans queue/ starvation time caused by jose_plus's crypto suites (keygen/sign/encrypt), which take ~55min per browser under dart2js's ~200x slower BigInt (#371) and starve other suites' loads while they run -- so any bound under that window is wrong. Remove the flag entirely; --timeout 60m and set -e stay, --ignore-timeouts stays removed (see the existing comment on why: a wedged Firefox launch silently hung for 4.5h without it, run 29118828702). Instead, tag jose_plus's crypto-executor suites (jwa/jwe/jwk/ecdh/jws/ jwt_test.dart) as slow-web-crypto via a new packages/jose_plus/ dart_test.yaml and exclude them from PULL-REQUEST web runs only, via a new WEB_EXCLUDE_TAGS env var threaded through melos's test:web:single into `dart test --exclude-tags`. VM runs always execute every tag (full PR coverage), and main-branch pushes keep full web crypto signal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * ci(tests): tag pss_eddsa suite as slow-web-crypto too Review finding on 3e070c4: pss_eddsa_jws_test.dart generates RSA-PSS keys (PS256/PS384/PS512) and signs/verifies -- measured 4:06 standalone under dart2js, the dominant slice of the remaining PR-web time -- but was missed by the six-file tag sweep. Tagged identically. PR-web chrome run drops from 6:01 (140 tests) to 2:23 (129 tests); VM still runs all 233. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * ci(tests): scope PR browser testing to platform-divergent packages With the crypto suites excluded, the measured remainder of the browser steps is dart2js COMPILATION: dart test compiles every test file separately (~30-60s each on CI runners), and the web sweep spans ~100 suites across six packages -- oidc_core alone is 60+ -- twice per browser per matrix leg. Most of that buys nothing per-PR: crypto_keys_plus, x509_plus, jose_plus and oidc_loopback_listener have zero web-conditional or js-interop lib code (verified by grep), so their browser runs duplicate the VM signal exactly. Only oidc_web_core (4 js-interop files) genuinely diverges; oidc_core's divergence is a single 6-line conditional stub. Pull requests now browser-test only oidc_web_core (new test:web:pr:chrome/firefox melos scripts, packageFilters scope); pushes to main keep the full sweep on both browsers. Expected PR web time: minutes instead of the better part of an hour per browser per leg. Full-platform coverage semantics are unchanged on main. Windows note: melos exec runs multi-line scripts through cmd.exe locally, so these scripts (like the existing test:web:single) are CI/sh-only; the scope filter was verified locally (resolves to exactly oidc_web_core) and the underlying per-package browser suites pass on both chrome and firefox. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BYvJ2hcGrcZbY3aKGy7JW2 * fix(ci): bound browser suite loads — a wedged load hangs forever by default package:test 1.31 puts no timeout on suite loads (suiteLoadTimeout defaults to Timeout.none), so a wedged browser suite load hangs until GitHub's 6h job kill. Run 29146393931 hit this twice on the PR-scoped steps: Firefox, then Chrome on the rerun, each finished one suite and stalled on the next load (63+ min on a ~20s step, past the --timeout 60m mark — proving no timer covers loads; --timeout only bounds tests). - test:web:pr:single: --suite-load-timeout 4m (three ~10s suite loads, no queue starvation) and drop --timeout 60m (sub-second tests; the 30s default also bounds a mid-test wedge). - test:web:single: --suite-load-timeout 2h on the full sweep — the load timer keeps running while suites queue behind jose's ~55min crypto executors, so the bound must clear that (a 4m bound false-failed 8 healthy suites, run 29140552735). 2h turns a wedge into a loud, rerunnable failure instead of a 6h hang. - Fix the stale comment claiming wedged loads fail loudly by default. Verified locally: the new PR-scope command runs oidc_web_core's 36 tests in 13s (chrome, coverage on, exit 0) under the locked test 1.31.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McVLYD2qr9r6Hec2MpKZaU * fix(ci): step-level timeout backstop on browser test steps The dart-level bounds added in e362d42 cover suite loads and test execution, but run 29148902320 proved a browser wedge can strike outside both: a Firefox PR-scope step hung 57+ minutes WITH the 4m suite-load bound active, so the hang was in browser launch, shutdown, or loader orchestration — phases no package:test timer covers. step timeout-minutes is the layer that bounds every hang shape: the runner kills the process tree and fails the step loudly, rerunnable instead of idling to GitHub's 6h job kill. PR-scope steps: 15m (observed 19-31s). Full-sweep steps: 150m (observed ~55-66min per browser on main pushes). Also corrects the suite count in the test:web:pr:single comment (oidc_web_core has five test suites, not three). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McVLYD2qr9r6Hec2MpKZaU * fix(ci): serialize PR-scope browser suites; the wedge is a post-success exit hang Log forensics across all three wedged runs (29146393931, 29148902320, 29150605150) show the same shape: every suite loads, every test passes, then dart test never exits — the runner idles from the last passing test until an outer timeout. Both browsers, both matrix legs. The earlier load-wedge reading was wrong: the hang is in the post-success phase (browser/suite teardown + Chrome coverage collection). dart-lang/test#2294 documents a multi-suite post-success exit hang where the runner reaches a successful end and a leaked async task keeps the process alive; --concurrency=1 is the reporter-verified workaround. - test:web:pr:single: add --concurrency=1 (five ~10s suites; serial costs seconds) and drop --coverage (removes the other post-success moving part; PR browser coverage is informational-only — codecov gets browser coverage from the full sweep on main pushes). - Keep --suite-load-timeout 4m and the step-level timeout-minutes backstop from the previous commits. Verified locally: the exact new command passes oidc_web_core's 36 tests (chrome, 1m26s, exit 0); 12 consecutive local runs of the OLD flags also pass on Windows — the hang reproduces only on CI Linux. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McVLYD2qr9r6Hec2MpKZaU --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #353.
The
test:web:singlemelos script's exit status was that of its last statement — theWITH_WASMif-block, which is 0 wheneverWITH_WASM != true— so web test failures were silently discarded andunit_testsstayed green while 27 web tests failed on main.set -erestores propagation (applied to the other chained melos scripts too).The hidden failures were
dart:iofixture reads, impossible on the web platform. The fixture-reading groups are now pinnedtestOn: 'vm'(x509_plus: 11 groups across 3 files, jose_plus: 1); pure-parsing groups keep their web coverage.Verified on real browsers: x509_plus 47/47, jose_plus 212/212, oidc_web_core 27/27 on both Chrome and Firefox (the oidc_web_core Firefox failures needed #360).