From f37368414e63ea0d19b552d82079970a9f2431e1 Mon Sep 17 00:00:00 2001 From: SATHVIK SVS Date: Sun, 9 Aug 2026 09:47:03 +0530 Subject: [PATCH 1/2] Pin the link-priority decision point so #200 cannot come back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #209 fixed issue #200 by giving the connection interval a real policy (`desiredLinkPriority`) and a single serialized applier. What is not pinned is that the engine keeps going through them. The original bug was not a wrong policy — it was no policy at all: a literal `requestConnectionPriority(ConnectionPriority.high)` sat in the connect path, ran once, and nothing stepped it back down. A refactor of that path can re-add exactly that line and every behavioural test here stays green, because `desiredLinkPriority` would still return the right answer to a caller that no longer exists. There is no BLE fake to assert against, so this greps instead, like no_debug_only_apis_test: * exactly one real requestConnectionPriority call in lib/, in the engine, inside _applyLinkPriority, taking its target from desiredLinkPriority — so no request can bypass the policy; * every ConnectionPriority literal inside that mapping switch; * exactly one desiredLinkPriority declaration, in sync_policy.dart, since the literal check is per-file and a second copy would satisfy it while the engine stopped being the single decision point. Comments AND string literals are stripped before matching. Both matter: this file names the offending API in prose, and the engine's own failure path logs `requestConnectionPriority(...) failed`, so a naive text count reports two calls where there is one. Mutation-tested rather than assumed: re-adding the original literal request trips the call-site and literal checks; a duplicate policy declaration trips the last one. --- test/link_priority_structural_test.dart | 175 ++++++++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 test/link_priority_structural_test.dart diff --git a/test/link_priority_structural_test.dart b/test/link_priority_structural_test.dart new file mode 100644 index 0000000..8463ca5 --- /dev/null +++ b/test/link_priority_structural_test.dart @@ -0,0 +1,175 @@ +// Issue #200, the other half: keep ONE decision point for the link interval. +// +// `link_priority_policy_test.dart` pins the stepping RULE. It cannot pin that +// the engine still routes through that rule. The original bug was not a wrong +// policy — it was no policy at all: a literal +// +// device.requestConnectionPriority( +// connectionPriorityRequest: ConnectionPriority.high) +// +// sat in the connect path, ran once, and nothing ever stepped it back down. A +// future refactor of that path can re-add exactly that line, and every +// behavioural test in this repo would stay green, because `desiredLinkPriority` +// would still return the right answer to a caller that no longer exists. +// +// There is no BLE fake here to assert against, so this greps instead — same +// approach as `no_debug_only_apis_test.dart`, for the same reason (the failure +// is invisible to any test that runs the code): +// +// 1. exactly ONE real `requestConnectionPriority` call in lib/, in the engine +// 2. that call sits inside `_applyLinkPriority`, and takes its target from +// `desiredLinkPriority` — so the request cannot bypass the policy +// 3. every `ConnectionPriority.` literal is inside that one method's +// mapping switch +// 4. exactly ONE `desiredLinkPriority` declaration, in `sync/sync_policy.dart` +// +// (4) matters because (3) is scoped per-file: a second copy of the policy in +// another lib/ file would carry its own literals and quietly satisfy (3) while +// the engine stopped being the single decision point. +// +// Comments AND string literals are stripped before matching. Both are load +// bearing: this file names the offending API in prose, and the engine's own +// failure log is `_log('requestConnectionPriority(...) failed: $e')` — a naive +// text count reports two calls where there is one. + +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; + +/// Blank out `//` comments and string literals, preserving line count so the +/// reported line numbers stay true. +List _codeLines(String source) => source.split('\n').map((line) { + final commentAt = line.indexOf('//'); + final noComment = commentAt == -1 ? line : line.substring(0, commentAt); + return noComment + .replaceAll(RegExp(r"r?'''(?:.|\n)*?'''"), "''") + .replaceAll(RegExp(r'r?"""(?:.|\n)*?"""'), '""') + .replaceAll(RegExp(r"r?'(?:\\.|[^'\\])*'"), "''") + .replaceAll(RegExp(r'r?"(?:\\.|[^"\\])*"'), '""'); + }).toList(); + +/// Every `.dart` file under `lib/`, as (path, code-only lines). +List>> _libSources() { + final lib = Directory('lib'); + expect(lib.existsSync(), isTrue, reason: 'run from the package root'); + final out = >>[]; + for (final entity in lib.listSync(recursive: true)) { + if (entity is! File || !entity.path.endsWith('.dart')) continue; + out.add(MapEntry(entity.path, _codeLines(entity.readAsStringSync()))); + } + out.sort((a, b) => a.key.compareTo(b.key)); + return out; +} + +/// Inclusive line range of the method whose signature contains [signature], +/// found by brace depth so it survives reformatting and nested blocks. +/// Returns null when the signature is absent. +({int start, int end})? _bodyRange(List lines, String signature) { + for (var i = 0; i < lines.length; i++) { + if (!lines[i].contains(signature)) continue; + var depth = 0; + var opened = false; + for (var j = i; j < lines.length; j++) { + for (final ch in lines[j].split('')) { + if (ch == '{') { + depth++; + opened = true; + } else if (ch == '}') { + depth--; + } + } + if (opened && depth == 0) return (start: i, end: j); + } + return null; // unbalanced — treat as not found rather than guessing + } + return null; +} + +void main() { + // A method call, not the API name appearing in prose or a log string. + final callSite = RegExp(r'\.requestConnectionPriority\s*\('); + final literal = RegExp(r'ConnectionPriority\.\w+'); + final policyDecl = RegExp(r'^\s*LinkPriority\s+desiredLinkPriority\s*\('); + + test('exactly one requestConnectionPriority call, and it is in the engine', + () { + final calls = []; + for (final entry in _libSources()) { + for (var i = 0; i < entry.value.length; i++) { + if (callSite.hasMatch(entry.value[i])) { + calls.add('${entry.key}:${i + 1}'); + } + } + } + expect( + calls, + hasLength(1), + reason: 'a second request site can bypass the policy. Found: $calls', + ); + expect(calls.single, startsWith('lib/ble/ble_engine.dart:')); + }); + + test('the request is inside _applyLinkPriority and takes desiredLinkPriority', + () { + final engine = _libSources() + .firstWhere((e) => e.key.endsWith('ble/ble_engine.dart')) + .value; + final range = _bodyRange(engine, 'Future _applyLinkPriority()'); + expect(range, isNotNull, + reason: '_applyLinkPriority must exist in ble/ble_engine.dart'); + + final body = engine.sublist(range!.start, range.end + 1); + expect( + body.any(callSite.hasMatch), + isTrue, + reason: 'the sole request must live in _applyLinkPriority', + ); + expect( + body.any((l) => l.contains('desiredLinkPriority(')), + isTrue, + reason: + 'the request must take its target from desiredLinkPriority, not a ' + 'literal or another selector — otherwise the policy is decorative', + ); + }); + + test('every ConnectionPriority literal is inside _applyLinkPriority', () { + final offences = []; + for (final entry in _libSources()) { + final range = _bodyRange(entry.value, 'Future _applyLinkPriority()'); + for (var i = 0; i < entry.value.length; i++) { + if (!literal.hasMatch(entry.value[i])) continue; + final inMapper = + range != null && i >= range.start && i <= range.end; + if (!inMapper) { + offences.add('${entry.key}:${i + 1} → ${entry.value[i].trim()}'); + } + } + } + expect( + offences, + isEmpty, + reason: 'a hard-coded ConnectionPriority outside the mapping switch is ' + 'how issue #200 shipped. Route it through desiredLinkPriority.', + ); + }); + + test('exactly one desiredLinkPriority declaration, in sync_policy.dart', () { + final decls = []; + for (final entry in _libSources()) { + for (var i = 0; i < entry.value.length; i++) { + if (policyDecl.hasMatch(entry.value[i])) { + decls.add('${entry.key}:${i + 1}'); + } + } + } + expect( + decls, + hasLength(1), + reason: 'a second policy copy would satisfy the per-file literal check ' + 'while the engine stopped being the single decision point. ' + 'Found: $decls', + ); + expect(decls.single, startsWith('lib/sync/sync_policy.dart:')); + }); +} From a7cd44d6e7804b667f4330c6ce361ffa6b177115 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sun, 9 Aug 2026 11:08:32 +0530 Subject: [PATCH 2/2] close the two holes the structural guard still left open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the guard as it stood. Both findings are mutation-proven, not argued: each mutation below was applied to a clean tree and the suite run. 1. THE MAPPING SWITCH WAS THE ONE UNGUARDED STEP. `_applyLinkPriority` was treated as a trusted region — test 3 exempted every `ConnectionPriority` literal inside it, and test 2 only asked that `desiredLinkPriority` appear somewhere in the body. So the last hop before the radio, LinkPriority -> ConnectionPriority, had no coverage at all. Mutating all three arms to `.high`: LinkPriority.balanced => ConnectionPriority.high, LinkPriority.lowPower => ConnectionPriority.high, is issue #200 exactly — the link pinned at ~11.25 ms overnight — and it passed 13/13. `desiredLinkPriority` still returned the right LinkPriority; nothing checked what it was translated into. Nothing in test/ referenced `ConnectionPriority` outside the structural file itself. A grep cannot close this: it cannot tell a correct arm from a wrong one. So the switch is lifted to `BleEngine.connectionPriorityFor`, @visibleForTesting, and `link_priority_policy_test.dart` asserts it arm by arm, plus that no two arms collapse onto one radio priority. The structural test now pins the whole chain instead of its ends — the request argument must be literally `connectionPriorityFor(want)`, and that `want` must be assigned from `desiredLinkPriority` — and adds a check that the mapper stays @visibleForTesting, since without it a refactor can make the arm-by-arm test vanish silently. 2. `_codeLines` STRIPPED `//` BEFORE STRING LITERALS, SO IT FAILED OPEN. `line.indexOf('//')` truncated at the `//` inside a string, discarding everything after it on that line. A real bypass call placed after a URL literal was invisible and the guard reported one call site, all green. It is live in the tree today: lib/ui/kit/route_map.dart:59 loses five brace pairs to this, and survives only because they happen to balance. It also missed `/* */` entirely (87 such lines in lib/, and a block comment naming the API produced two spurious failures), and its `'''` patterns ran inside a per-line `.map`, where `\n` can never match — dead code against the ~80 multi-line strings in lib/. Replaced with a stateful scanner in test/support/dart_source.dart that also tracks `${...}` interpolation, so `'${m['k']}'` does not end the string early. Line count and column widths are preserved, so reported positions stay true. `no_debug_only_apis_test.dart` now uses it too rather than keeping a second, differently-wrong copy. 12 regression cases in dart_source_test.dart, one per way the old helper got it wrong. Verified by mutation — 8 cases, each on a clean tree: all three mapping arms to .high fails (was: passed) bypass call behind a '//' in string fails (was: passed) literal passed to the request fails original literal request at connect fails second desiredLinkPriority decl fails second connectionPriorityFor decl fails mapper loses @visibleForTesting fails block comment naming the API passes (was: 2 false failures) flutter analyze lib test clean. Full suite +1501, 0 failures. Production change is the extraction only; no behaviour change, no kAlgoVersion bump. --- lib/ble/ble_engine.dart | 22 +++- test/dart_source_test.dart | 105 +++++++++++++++++ test/link_priority_policy_test.dart | 51 +++++++++ test/link_priority_structural_test.dart | 137 +++++++++++++++------- test/no_debug_only_apis_test.dart | 25 ++-- test/support/dart_source.dart | 145 ++++++++++++++++++++++++ 6 files changed, 421 insertions(+), 64 deletions(-) create mode 100644 test/dart_source_test.dart create mode 100644 test/support/dart_source.dart diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index 63a0560..a1303a8 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -641,6 +641,22 @@ class BleEngine { hasLiveConsumer: _liveEnabled && !_liveHrOnly, ); + /// The last hop: the policy's [LinkPriority] as the radio's own enum. + /// + /// Lifted out of [_applyLinkPriority] because inline it was the one step + /// nothing covered. `desiredLinkPriority` could keep returning exactly the + /// right answer while every arm here mapped to `ConnectionPriority.high` — + /// which IS issue #200, the link pinned at ~11.25 ms overnight — and the + /// whole suite stayed green. Arm-by-arm coverage is in + /// `link_priority_policy_test.dart`. + @visibleForTesting + static ConnectionPriority connectionPriorityFor(LinkPriority want) => + switch (want) { + LinkPriority.high => ConnectionPriority.high, + LinkPriority.balanced => ConnectionPriority.balanced, + LinkPriority.lowPower => ConnectionPriority.lowPower, + }; + @visibleForTesting void debugBeginConnectSetup() => _connectSetup = true; @@ -685,11 +701,7 @@ class BleEngine { final generation = _linkGeneration; try { await session.device.requestConnectionPriority( - connectionPriorityRequest: switch (want) { - LinkPriority.high => ConnectionPriority.high, - LinkPriority.balanced => ConnectionPriority.balanced, - LinkPriority.lowPower => ConnectionPriority.lowPower, - }, + connectionPriorityRequest: connectionPriorityFor(want), ); // Only remember it if the link we asked is still the live one. A // teardown during the await clears `_appliedPriority` precisely so diff --git a/test/dart_source_test.dart b/test/dart_source_test.dart new file mode 100644 index 0000000..a29ef48 --- /dev/null +++ b/test/dart_source_test.dart @@ -0,0 +1,105 @@ +// Regression cases for the scanner the structural tests are built on. +// +// Every case here is a way the previous per-line regex helper got it wrong. +// They matter because when this primitive fails, it fails invisibly: the grep +// test it feeds goes green either way. + +import 'package:flutter_test/flutter_test.dart'; + +import 'support/dart_source.dart'; + +void main() { + test('line count and column positions survive', () { + const src = "a();\n// gone\nb('str');\n"; + final lines = codeLines(src); + expect(lines, hasLength(4)); + expect(lines[0], 'a();'); + expect(lines[1].trim(), isEmpty); + // The literal goes, quotes included, but its width is kept so columns + // still line up with the original source. + expect(lines[2], 'b( );'); + expect(lines[2], hasLength("b('str');".length)); + }); + + test("a '//' inside a string does not truncate the rest of the line", () { + // The bug: stripping `//` before strings ate everything after the URL, + // including a real call the guard exists to catch. + const src = "const u = 'https://x/y'; d.requestConnectionPriority(1);"; + final code = stripCommentsAndStrings(src); + expect(code, contains('requestConnectionPriority')); + expect(code, isNot(contains('https'))); + }); + + test('braces inside a string are removed, not counted', () { + // lib/ui/kit/route_map.dart is a live instance of this. + const src = "const t = 'https://{s}.tiles/{z}/{x}/{y}{r}.png';"; + final code = stripCommentsAndStrings(src); + expect(code.contains('{'), isFalse); + expect(code.contains('}'), isFalse); + }); + + test('block comments are stripped, and they nest', () { + const src = 'a(); /* x /* y */ z */ b();'; + final code = stripCommentsAndStrings(src); + expect(code, contains('a();')); + expect(code, contains('b();')); + expect(code, isNot(contains('x'))); + expect(code, isNot(contains('z'))); + }); + + test('a multi-line block comment keeps its newlines', () { + const src = 'a();\n/* one\n two\n */\nb();'; + final lines = codeLines(src); + expect(lines, hasLength(5)); + expect(lines[4], 'b();'); + expect(lines[1].trim(), isEmpty); + expect(lines[2].trim(), isEmpty); + }); + + test('a token named in a block comment is not a match', () { + const src = + '/* used to call d.requestConnectionPriority(x) here */\nok();'; + expect( + stripCommentsAndStrings(src), + isNot(contains('requestConnectionPriority')), + ); + }); + + test('triple-quoted strings are stripped across lines', () { + // No per-line regex can do this, and lib/ has ~80 such lines. + const src = "final q = '''\nrequestConnectionPriority(\n{{{\n''';\nok();"; + final code = stripCommentsAndStrings(src); + expect(code, isNot(contains('requestConnectionPriority'))); + expect(code.contains('{'), isFalse); + expect(code, contains('ok();')); + expect(codeLines(src), hasLength(5)); + }); + + test('interpolation containing a quote does not end the string early', () { + const src = "_log('a \${m['k']} b'); real();"; + final code = stripCommentsAndStrings(src); + expect(code, contains('_log(')); + expect(code, contains('real();')); + expect(code, isNot(contains('k'))); + expect(code.contains('{'), isFalse); + }); + + test('a string inside interpolation can itself hold a comment marker', () { + const src = "final s = '\${f('//')} tail'; kept();"; + expect(stripCommentsAndStrings(src), contains('kept();')); + }); + + test('escapes do not terminate a string, and raw strings ignore them', () { + expect(stripCommentsAndStrings(r"var a = 'x\'y'; z();"), contains('z();')); + expect(stripCommentsAndStrings(r"var a = r'x\'; z();"), contains('z();')); + }); + + test('the engine failure log is not counted as a call site', () { + // The concrete reason the structural test needs string stripping at all. + const src = r"_log('requestConnectionPriority(${want.name}) failed: $e');"; + expect( + stripCommentsAndStrings(src), + isNot(contains('requestConnectionPriority')), + ); + }); +} diff --git a/test/link_priority_policy_test.dart b/test/link_priority_policy_test.dart index d3275b8..9cba360 100644 --- a/test/link_priority_policy_test.dart +++ b/test/link_priority_policy_test.dart @@ -10,6 +10,7 @@ // offload always runs at the fast interval, whatever else is going on, because // throughput during a drain is what the fast interval was for. +import 'package:flutter_blue_plus/flutter_blue_plus.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:openstrap_edge/ble/ble_engine.dart'; import 'package:openstrap_edge/sync/sync_policy.dart'; @@ -84,6 +85,56 @@ void main() { } }); + group('the policy survives the last hop to the radio', () { + // The gap this closes: `desiredLinkPriority` can keep returning exactly + // the right LinkPriority while the switch that turns it into the radio's + // own enum maps every arm to `ConnectionPriority.high`. That IS issue #200 + // — the link pinned at ~11.25 ms overnight — and with the mapping inline + // and untested, the whole suite stayed green through it. Verified by + // mutation: all three arms to `.high` passed 13/13 before this existed. + const expected = { + LinkPriority.high: ConnectionPriority.high, + LinkPriority.balanced: ConnectionPriority.balanced, + LinkPriority.lowPower: ConnectionPriority.lowPower, + }; + + test('every arm maps to its own priority, none of them to high', () { + for (final want in LinkPriority.values) { + expect( + BleEngine.connectionPriorityFor(want), + expected[want], + reason: '${want.name} must not be silently promoted', + ); + } + }); + + test('the mapping is exhaustive, and no two arms collapse', () { + // A LinkPriority added to the enum has to be given a mapping here rather + // than inheriting whatever the switch falls through to. + expect(expected.keys, unorderedEquals(LinkPriority.values)); + expect( + LinkPriority.values.map(BleEngine.connectionPriorityFor).toSet(), + hasLength(LinkPriority.values.length), + reason: 'two link priorities collapsing to one radio priority means ' + 'one of the steps is not actually a step', + ); + }); + + test('the overnight state reaches the radio as lowPower', () { + // End to end through both halves: the rule, then the mapping. + expect( + BleEngine.connectionPriorityFor( + desiredLinkPriority( + offloadActive: false, + background: true, + hasLiveConsumer: false, + ), + ), + ConnectionPriority.lowPower, + ); + }); + }); + test('the battery poll is minutes apart, not seconds', () { // It rode the 30 s keep-alive tick: 2,880 radio round-trips a day for a // display value that changes a handful of times. diff --git a/test/link_priority_structural_test.dart b/test/link_priority_structural_test.dart index 8463ca5..f7205e5 100644 --- a/test/link_priority_structural_test.dart +++ b/test/link_priority_structural_test.dart @@ -1,7 +1,8 @@ // Issue #200, the other half: keep ONE decision point for the link interval. // -// `link_priority_policy_test.dart` pins the stepping RULE. It cannot pin that -// the engine still routes through that rule. The original bug was not a wrong +// `link_priority_policy_test.dart` pins the stepping RULE, and the +// LinkPriority → ConnectionPriority mapping arm by arm. Neither can pin that +// the engine still routes THROUGH them. The original bug was not a wrong // policy — it was no policy at all: a literal // // device.requestConnectionPriority( @@ -17,36 +18,39 @@ // is invisible to any test that runs the code): // // 1. exactly ONE real `requestConnectionPriority` call in lib/, in the engine -// 2. that call sits inside `_applyLinkPriority`, and takes its target from -// `desiredLinkPriority` — so the request cannot bypass the policy -// 3. every `ConnectionPriority.` literal is inside that one method's -// mapping switch -// 4. exactly ONE `desiredLinkPriority` declaration, in `sync/sync_policy.dart` +// 2. that call sits inside `_applyLinkPriority`, its argument is exactly +// `connectionPriorityFor(want)`, and that `want` is assigned from +// `desiredLinkPriority` — so no link in the chain can be short-circuited +// 3. every `ConnectionPriority.` literal is inside the mapper +// 4. the mapper is declared once, in the engine, and stays @visibleForTesting +// 5. exactly ONE `desiredLinkPriority` declaration, in `sync/sync_policy.dart` // -// (4) matters because (3) is scoped per-file: a second copy of the policy in +// (5) matters because (3) is scoped per-file: a second copy of the policy in // another lib/ file would carry its own literals and quietly satisfy (3) while // the engine stopped being the single decision point. // -// Comments AND string literals are stripped before matching. Both are load -// bearing: this file names the offending API in prose, and the engine's own -// failure log is `_log('requestConnectionPriority(...) failed: $e')` — a naive -// text count reports two calls where there is one. +// WHAT THIS DELIBERATELY DOES NOT DO: assert the mapping is CORRECT. A grep +// cannot tell `LinkPriority.lowPower => ConnectionPriority.lowPower` from +// `=> ConnectionPriority.high`, and the latter is #200 restored — so that is +// `link_priority_policy_test.dart`'s job, and (4) is what keeps it able to do +// it. The two files are only jointly sufficient. +// +// Comments and string literals are stripped by `support/dart_source.dart`; see +// there for why that is subtler than a regex. Both matter here: this file names +// the offending API in prose, and the engine's own failure log is +// `_log('requestConnectionPriority(...) failed: $e')`, so a naive text count +// reports two calls where there is one. import 'dart:io'; import 'package:flutter_test/flutter_test.dart'; -/// Blank out `//` comments and string literals, preserving line count so the -/// reported line numbers stay true. -List _codeLines(String source) => source.split('\n').map((line) { - final commentAt = line.indexOf('//'); - final noComment = commentAt == -1 ? line : line.substring(0, commentAt); - return noComment - .replaceAll(RegExp(r"r?'''(?:.|\n)*?'''"), "''") - .replaceAll(RegExp(r'r?"""(?:.|\n)*?"""'), '""') - .replaceAll(RegExp(r"r?'(?:\\.|[^'\\])*'"), "''") - .replaceAll(RegExp(r'r?"(?:\\.|[^"\\])*"'), '""'); - }).toList(); +import 'support/dart_source.dart'; + +/// The two members this test pins, matched as written. +const _applySig = 'Future _applyLinkPriority()'; +const _mapperSig = + 'ConnectionPriority connectionPriorityFor(LinkPriority want)'; /// Every `.dart` file under `lib/`, as (path, code-only lines). List>> _libSources() { @@ -55,14 +59,17 @@ List>> _libSources() { final out = >>[]; for (final entity in lib.listSync(recursive: true)) { if (entity is! File || !entity.path.endsWith('.dart')) continue; - out.add(MapEntry(entity.path, _codeLines(entity.readAsStringSync()))); + out.add(MapEntry(entity.path, codeLines(entity.readAsStringSync()))); } out.sort((a, b) => a.key.compareTo(b.key)); return out; } -/// Inclusive line range of the method whose signature contains [signature], +/// Inclusive line range of the member whose signature contains [signature], /// found by brace depth so it survives reformatting and nested blocks. +/// +/// Handles a braced body and an `=>` body closed by `;`, so the mapper can be +/// an expression member without silently vacating the check. /// Returns null when the signature is absent. ({int start, int end})? _bodyRange(List lines, String signature) { for (var i = 0; i < lines.length; i++) { @@ -79,12 +86,20 @@ List>> _libSources() { } } if (opened && depth == 0) return (start: i, end: j); + // `=> …;` with no braces at all: the member ends at the first `;`. + if (!opened && depth == 0 && lines[j].contains(';')) { + return (start: i, end: j); + } } return null; // unbalanced — treat as not found rather than guessing } return null; } +List _engine() => _libSources() + .firstWhere((e) => e.key.endsWith('ble/ble_engine.dart')) + .value; + void main() { // A method call, not the API name appearing in prose or a log string. final callSite = RegExp(r'\.requestConnectionPriority\s*\('); @@ -109,38 +124,49 @@ void main() { expect(calls.single, startsWith('lib/ble/ble_engine.dart:')); }); - test('the request is inside _applyLinkPriority and takes desiredLinkPriority', - () { - final engine = _libSources() - .firstWhere((e) => e.key.endsWith('ble/ble_engine.dart')) - .value; - final range = _bodyRange(engine, 'Future _applyLinkPriority()'); + test('the request is in _applyLinkPriority, fed by the whole chain', () { + final engine = _engine(); + final range = _bodyRange(engine, _applySig); expect(range, isNotNull, - reason: '_applyLinkPriority must exist in ble/ble_engine.dart'); + reason: '$_applySig must exist in ble/ble_engine.dart'); + + // Joined, because the call and its argument sit on different lines. + final body = engine.sublist(range!.start, range.end + 1).join('\n'); - final body = engine.sublist(range!.start, range.end + 1); expect( - body.any(callSite.hasMatch), + callSite.hasMatch(body), isTrue, reason: 'the sole request must live in _applyLinkPriority', ); + // The argument is the mapper's return value and nothing else. Merely + // requiring `connectionPriorityFor` SOMEWHERE in the body would pass a + // body that computes it and then requests a literal anyway. expect( - body.any((l) => l.contains('desiredLinkPriority(')), + RegExp(r'connectionPriorityRequest:\s*connectionPriorityFor\(') + .hasMatch(body), isTrue, - reason: - 'the request must take its target from desiredLinkPriority, not a ' - 'literal or another selector — otherwise the policy is decorative', + reason: 'the request argument must be connectionPriorityFor(...) — not ' + 'a literal and not another selector, or the policy is decorative', + ); + expect( + RegExp(r'\bwant\s*=\s*desiredLinkPriority\(').hasMatch(body), + isTrue, + reason: "and the mapper's input must come from desiredLinkPriority", + ); + expect( + RegExp(r'connectionPriorityFor\(\s*want\s*\)').hasMatch(body), + isTrue, + reason: 'the value handed to the mapper must be that same `want`', ); }); - test('every ConnectionPriority literal is inside _applyLinkPriority', () { + test('every ConnectionPriority literal is inside the mapper', () { final offences = []; for (final entry in _libSources()) { - final range = _bodyRange(entry.value, 'Future _applyLinkPriority()'); + final range = _bodyRange(entry.value, _mapperSig); for (var i = 0; i < entry.value.length; i++) { if (!literal.hasMatch(entry.value[i])) continue; - final inMapper = - range != null && i >= range.start && i <= range.end; + final inMapper = range != null && i >= range.start && i <= range.end; if (!inMapper) { offences.add('${entry.key}:${i + 1} → ${entry.value[i].trim()}'); } @@ -154,6 +180,33 @@ void main() { ); }); + test('the mapper is declared once, in the engine, and stays testable', () { + final decls = []; + for (final entry in _libSources()) { + for (var i = 0; i < entry.value.length; i++) { + if (entry.value[i].contains(_mapperSig)) { + decls.add('${entry.key}:${i + 1}'); + } + } + } + expect(decls, hasLength(1), reason: 'Found: $decls'); + expect(decls.single, startsWith('lib/ble/ble_engine.dart:')); + + // Without @visibleForTesting a refactor can make it private, and the + // arm-by-arm coverage in link_priority_policy_test.dart — the only thing + // standing between a future edit and #200 itself — quietly disappears. + final engine = _engine(); + final at = int.parse(decls.single.split(':').last) - 1; + expect( + engine + .sublist((at - 3).clamp(0, at), at) + .any((l) => l.contains('@visibleForTesting')), + isTrue, + reason: 'connectionPriorityFor must stay @visibleForTesting so the ' + 'mapping keeps its arm-by-arm test', + ); + }); + test('exactly one desiredLinkPriority declaration, in sync_policy.dart', () { final decls = []; for (final entry in _libSources()) { diff --git a/test/no_debug_only_apis_test.dart b/test/no_debug_only_apis_test.dart index 676c9e4..8422fe6 100644 --- a/test/no_debug_only_apis_test.dart +++ b/test/no_debug_only_apis_test.dart @@ -38,6 +38,8 @@ import 'dart:io'; import 'package:flutter_test/flutter_test.dart'; +import 'support/dart_source.dart'; + /// Getters whose value only exists when asserts are enabled. const _assertStrippedMembers = [ 'debugNeedsPaint', @@ -45,19 +47,6 @@ const _assertStrippedMembers = [ 'debugNeedsCompositedLayerUpdate', ]; -/// Strip `//` line comments and `/* */` blocks so a member named in an -/// explanatory comment (this file's own history, for one) isn't a false hit. -String _stripComments(String source) { - final noBlocks = source.replaceAll(RegExp(r'/\*.*?\*/', dotAll: true), ''); - return noBlocks - .split('\n') - .map((l) { - final i = l.indexOf('//'); - return i == -1 ? l : l.substring(0, i); - }) - .join('\n'); -} - void main() { test('no assert-stripped Flutter APIs are called in lib/', () { final lib = Directory('lib'); @@ -66,7 +55,7 @@ void main() { final offences = []; for (final entity in lib.listSync(recursive: true)) { if (entity is! File || !entity.path.endsWith('.dart')) continue; - final code = _stripComments(entity.readAsStringSync()); + final code = stripCommentsAndStrings(entity.readAsStringSync()); final lines = code.split('\n'); for (var i = 0; i < lines.length; i++) { for (final member in _assertStrippedMembers) { @@ -95,17 +84,19 @@ void main() { expect(_assertStrippedMembers, contains('debugNeedsPaint')); }); - test('the comment stripper does not hide a real call', () { + test('the shared source stripper does not hide a real call', () { const sample = ''' // boundary.debugNeedsPaint is mentioned here in prose final x = boundary.debugNeedsPaint; '''; - final stripped = _stripComments(sample); + final stripped = stripCommentsAndStrings(sample); expect(stripped.contains('.debugNeedsPaint'), isTrue, reason: 'the real call on line 2 must survive stripping'); expect('\n'.allMatches(stripped).length, greaterThan(1)); // And a comment-only mention must NOT trip it. const commentOnly = '// see boundary.debugNeedsPaint for why'; - expect(_stripComments(commentOnly).contains('.debugNeedsPaint'), isFalse); + expect( + stripCommentsAndStrings(commentOnly).contains('.debugNeedsPaint'), + isFalse); }); } diff --git a/test/support/dart_source.dart b/test/support/dart_source.dart new file mode 100644 index 0000000..6e2dacf --- /dev/null +++ b/test/support/dart_source.dart @@ -0,0 +1,145 @@ +// Shared source scanner for the structural ("grep") tests. +// +// Several tests in this suite assert things about the SHAPE of `lib/` that no +// test running the code can see — `no_debug_only_apis_test.dart` (an API that +// only misbehaves in release), `link_priority_structural_test.dart` (a call +// site that must not be duplicated). All of them need the same primitive: the +// source with its comments and string literals removed, so a token named in +// prose or in a log message is not mistaken for a call. +// +// Getting that primitive right is fiddly enough to be worth doing once: +// +// * `//` must be stripped AFTER string literals, not before. Stripping it +// first truncates the line at the `//` inside `'https://…'` and silently +// discards everything after it — including, in the worst case, a real call +// the test exists to catch. `lib/ui/kit/route_map.dart` has exactly such a +// URL, and it carries five brace pairs that a naive strip throws away. +// * `/* … */` must be handled, and it nests in Dart. +// * `'''…'''` spans lines, so no per-line regex can consume one — `lib/` has +// ~80 lines of them. +// * `'${foo('bar')}'` puts a quote inside a string, so the scanner has to +// track interpolation rather than pair quotes naively. +// +// Line count is preserved exactly: every removed character becomes a space, +// every newline stays a newline, so reported line numbers stay true. + +/// One string literal we are currently inside. +/// +/// [depth] is 0 while lexing the string body and counts unclosed braces once +/// we step into a `${…}` interpolation — which is code, and may open further +/// strings of its own. +class _StringFrame { + _StringFrame(this.quote, this.triple, this.raw); + + final String quote; + final bool triple; + final bool raw; + int depth = 0; +} + +/// [source] with every comment and string literal blanked to spaces. +/// +/// Interpolated code is blanked along with the string that contains it: it is +/// still inside a literal, and removing it keeps braces balanced for callers +/// that locate a method body by brace depth. +String stripCommentsAndStrings(String source) { + final out = StringBuffer(); + final stack = <_StringFrame>[]; + final n = source.length; + var i = 0; + + void blank(int count) { + for (var k = 0; k < count && i + k < n; k++) { + out.write(source[i + k] == '\n' ? '\n' : ' '); + } + i += count; + } + + while (i < n) { + final frame = stack.isEmpty ? null : stack.last; + + // ── inside a string body ──────────────────────────────────────────────── + if (frame != null && frame.depth == 0) { + if (!frame.raw && source[i] == r'\') { + blank(2); + continue; + } + if (!frame.raw && source.startsWith(r'${', i)) { + frame.depth = 1; // step into interpolated code + blank(2); + continue; + } + final close = frame.triple ? frame.quote * 3 : frame.quote; + if (source.startsWith(close, i)) { + stack.removeLast(); + blank(close.length); + continue; + } + blank(1); + continue; + } + + // ── code: either top level, or inside a `${…}` ────────────────────────── + if (source.startsWith('//', i)) { + final nl = source.indexOf('\n', i); + blank((nl == -1 ? n : nl) - i); // leave the newline itself + continue; + } + if (source.startsWith('/*', i)) { + var depth = 0; + var j = i; + while (j < n) { + if (source.startsWith('/*', j)) { + depth++; + j += 2; + } else if (source.startsWith('*/', j)) { + depth--; + j += 2; + if (depth == 0) break; + } else { + j++; + } + } + blank(j - i); + continue; + } + + // A string start, optionally raw (`r'…'`). + var q = i; + var raw = false; + if (source[i] == 'r' && + i + 1 < n && + (source[i + 1] == "'" || source[i + 1] == '"')) { + raw = true; + q = i + 1; + } + if (source[q] == "'" || source[q] == '"') { + final quote = source[q]; + final triple = source.startsWith(quote * 3, q); + stack.add(_StringFrame(quote, triple, raw)); + blank((q - i) + (triple ? 3 : 1)); + continue; + } + + if (frame != null) { + // Interpolated code — track braces so we know where it ends, but blank + // it, because it is part of the literal. + if (source[i] == '{') { + frame.depth++; + } else if (source[i] == '}') { + frame.depth--; // 0 ⇒ back to the string body + } + blank(1); + continue; + } + + out.write(source[i]); + i++; + } + + return out.toString(); +} + +/// [stripCommentsAndStrings], as lines. Index `i` is source line `i + 1`. +List codeLines(String source) => + stripCommentsAndStrings(source).split('\n');