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 new file mode 100644 index 0000000..f7205e5 --- /dev/null +++ b/test/link_priority_structural_test.dart @@ -0,0 +1,228 @@ +// Issue #200, the other half: keep ONE decision point for the link interval. +// +// `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( +// 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`, 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` +// +// (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. +// +// 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'; + +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() { + 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 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++) { + 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); + // `=> …;` 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*\('); + 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 in _applyLinkPriority, fed by the whole chain', () { + final engine = _engine(); + final range = _bodyRange(engine, _applySig); + expect(range, isNotNull, + 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'); + + expect( + 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( + RegExp(r'connectionPriorityRequest:\s*connectionPriorityFor\(') + .hasMatch(body), + isTrue, + 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 the mapper', () { + final offences = []; + for (final entry in _libSources()) { + 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; + 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('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()) { + 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:')); + }); +} 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');