From 95c358cb5c8af57d51f9e6fabf1f5885b55aeb0d Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Fri, 14 Aug 2026 16:44:32 +0200 Subject: [PATCH] Split sync_service_test.dart; all code and tests now under the cap 356 -> sync_service_test (133) + sync_service_revision_test (96) + sync_service_fakes (163). The underscores came off every shared fake: Dart privacy is library-scoped, so `_FakeRemote` declared in one file is invisible to the next. `_peerLog` became `buildPeerLog` rather than `peerLog`, because two tests declare a local variable of that name and the helper would have been shadowed by its own caller. The `_revisionTrackingTests()` indirection is gone -- that function existed only to keep main() readable, which is now the file boundary's job. Verified: 313 tests pass, 100% coverage. Only CLAUDE.md and docs/desktop-performance-findings.md remain over 250 lines. Co-Authored-By: Claude Opus 4.8 --- pubspec.yaml | 2 +- test/sync_service_fakes.dart | 161 ++++++++++++++++ test/sync_service_revision_test.dart | 95 ++++++++++ test/sync_service_test.dart | 266 +++------------------------ 4 files changed, 278 insertions(+), 246 deletions(-) create mode 100644 test/sync_service_fakes.dart create mode 100644 test/sync_service_revision_test.dart diff --git a/pubspec.yaml b/pubspec.yaml index 77640d7..4df39bf 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -16,7 +16,7 @@ publish_to: 'none' # Remove this line if you wish to publish to pub.dev # https://developer.apple.com/library/archive/documentation/General/Reference/InfoPlistKeyReference/Articles/CoreFoundationKeys.html # In Windows, build-name is used as the major, minor, and patch parts # of the product and file versions while build-number is used as the build suffix. -version: 1.0.16+1 +version: 1.0.17+1 environment: sdk: ^3.12.2 diff --git a/test/sync_service_fakes.dart b/test/sync_service_fakes.dart new file mode 100644 index 0000000..a6aa848 --- /dev/null +++ b/test/sync_service_fakes.dart @@ -0,0 +1,161 @@ +/// The GitHub/remote fakes and note fixtures shared by the sync_service +/// test files. +/// +/// `sync_service_test.dart` was split for the 250-line cap. The underscores +/// came off every name here: Dart privacy is library-scoped, so a `_Fake` +/// declared in one file is invisible to the next. +/// +/// Deliberately NOT named `*_test.dart`: the runner would collect it and +/// fail on the missing `main()`. +library; + +import 'dart:convert'; + +import 'package:crdt_sync/crdt_sync.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:http/http.dart' as http; +import 'package:http/testing.dart' as http_testing; +import 'package:todo/data/note.dart'; +import 'package:todo/data/note_repository.dart'; +import 'package:todo/sync/sync_service.dart'; + +http.Response response(int statusCode, [Object? jsonBody]) => + http.Response(jsonEncode(jsonBody ?? {}), statusCode); + +http.Response fileWith(String text, {String? sha}) => + response(200, {'content': base64.encode(utf8.encode(text)), 'sha': ?sha}); + +class PutCall { + PutCall(this.path, this.body); + final String path; + final Map body; +} + +/// Mock GitHub Contents API router matching crdt_sync's client: GET +/// `.../contents/` returns [contentResponses]`[key]` (404 if absent), the +/// bare repo-existence GET always succeeds, and every PUT is recorded. +({http.Client httpClient, List putCalls}) mockGitHub({ + Map contentResponses = const {}, +}) { + final putCalls = []; + final client = http_testing.MockClient((request) async { + final path = request.url.path; + if (!path.contains('/contents/')) return response(200); // repo-exists + final key = path.split('/contents/').last; + if (request.method == 'PUT') { + putCalls.add( + PutCall(key, jsonDecode(request.body) as Map), + ); + return response(200); + } + return contentResponses[key] ?? response(404); + }); + return (httpClient: client, putCalls: putCalls); +} + +GitHubClient github(http.Client mock) => + GitHubClient(owner: 'o', repo: 'r', token: 't', httpClient: mock); + +Note note(String id, String text) => Note( + id: id, + text: text, + priority: Priority.medium, + status: Status.todo, + createdAt: DateTime(2026, 6, 15), + updatedAt: DateTime(2026, 6, 15), +); + +/// An in-memory [RemoteStore] that also serves revision maps in one read, +/// standing in for Firebase. Counts reads so tests can assert on *traffic* +/// rather than on the merge result. +class FakeRemote implements RemoteStore, BulkMapReader { + FakeRemote(this.files); + + final Map files; + final List reads = []; + final List writes = []; + + @override + Future> listDirectory(String path) async => files.keys + .where((k) => k.startsWith('$path/')) + .map((k) => k.substring(path.length + 1).split('/').first) + .toSet() + .toList(); + + @override + Future getFileText(String path) async { + reads.add(path); + return files[path]; + } + + @override + Future putFileText( + String path, + String text, { + required String message, + }) async { + writes.add(path); + files[path] = text; + } + + @override + Future> getStringMap(String path) async => { + for (final e in files.entries) + if (e.key.startsWith('$path/')) e.key.substring(path.length + 1): e.value, + }; + + @override + Future deleteFile(String path, {String message = ''}) async => + files.remove(path); + + @override + Future canAccessRemote() async => true; + + @override + void close() {} +} + +/// A [RemoteStore] with no bulk-map read, standing in for GitHub. +/// +/// Delegates rather than extends: a subclass of [FakeRemote] would inherit +/// `BulkMapReader`, the very capability this fake exists to lack. +class FakeRemoteWithoutBulkRead implements RemoteStore { + FakeRemoteWithoutBulkRead(Map files) + : _inner = FakeRemote(files); + + final FakeRemote _inner; + + List get reads => _inner.reads; + + @override + Future> listDirectory(String path) => _inner.listDirectory(path); + + @override + Future getFileText(String path) => _inner.getFileText(path); + + @override + Future putFileText( + String path, + String text, { + required String message, + }) => _inner.putFileText(path, text, message: message); + + @override + Future deleteFile(String path, {String message = ''}) => + _inner.deleteFile(path, message: message); + + @override + Future canAccessRemote() => _inner.canAccessRemote(); + + @override + void close() => _inner.close(); +} + +/// Builds a one-note log as it would arrive from another device. +Future buildPeerLog({String title = 'from phone'}) async { + final peer = await NoteRepository.openInMemory(nodeId: 'phone'); + await peer.upsert(note('peer-note', title)); + final log = peer.exportLog(); + await peer.close(); + return log; +} diff --git a/test/sync_service_revision_test.dart b/test/sync_service_revision_test.dart new file mode 100644 index 0000000..a228226 --- /dev/null +++ b/test/sync_service_revision_test.dart @@ -0,0 +1,95 @@ +import 'dart:convert'; + +import 'package:crdt_sync/crdt_sync.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:http/http.dart' as http; +import 'package:http/testing.dart' as http_testing; +import 'package:todo/data/note.dart'; +import 'package:todo/data/note_repository.dart'; +import 'package:todo/sync/sync_service.dart'; + +import 'sync_service_fakes.dart'; + +void main() { + group('revision tracking', () { + late NoteRepository repo; + + setUp(() async { + repo = await NoteRepository.openInMemory(nodeId: 'me'); + }); + + tearDown(() async => repo.close()); + + test('skips a peer whose revision has not changed', () async { + // The saving the free-tier budget depends on: an unchanged ~150 KB peer + // log must not be downloaded again. + final peerLog = logToJson(await buildPeerLog()); + final remote = FakeRemote({ + 'todo-sync/notes/phone.json': peerLog, + 'todo-sync/revs/phone': revisionOf(peerLog), + }); + final store = InMemorySyncStateStore(); + + await SyncService(stateStore: store).sync(repo, remote); + remote.reads.clear(); + final second = await SyncService(stateStore: store).sync(repo, remote); + + expect(remote.reads, isNot(contains('todo-sync/notes/phone.json'))); + expect(second.skippedUnchanged, 1); + expect(second.mergedDevices, 0); + }); + + test('downloads again once the peer publishes a new revision', () async { + final first = logToJson(await buildPeerLog()); + final remote = FakeRemote({ + 'todo-sync/notes/phone.json': first, + 'todo-sync/revs/phone': revisionOf(first), + }); + final store = InMemorySyncStateStore(); + await SyncService(stateStore: store).sync(repo, remote); + remote.reads.clear(); + + final changed = logToJson(await buildPeerLog(title: 'changed')); + remote.files['todo-sync/notes/phone.json'] = changed; + remote.files['todo-sync/revs/phone'] = revisionOf(changed); + final second = await SyncService(stateStore: store).sync(repo, remote); + + expect(remote.reads, contains('todo-sync/notes/phone.json')); + expect(second.mergedDevices, 1); + }); + + test('suppresses an unchanged push and publishes a revision', () async { + final remote = FakeRemote({}); + final store = InMemorySyncStateStore(); + + final first = await SyncService(stateStore: store).sync(repo, remote); + expect(first.pushed, isTrue); + expect(remote.writes, [ + 'todo-sync/notes/me.json', + 'todo-sync/revs/me', + ]); + remote.writes.clear(); + + final second = await SyncService(stateStore: store).sync(repo, remote); + + expect(second.pushed, isFalse); + expect(remote.writes, isEmpty); + }); + + test('still syncs on a backend without a bulk-map read', () async { + // GitHub has no cheap revision map; correctness must not depend on the + // optimisation being available. + final peerLog = logToJson(await buildPeerLog()); + final remote = FakeRemoteWithoutBulkRead({ + 'todo-sync/notes/phone.json': peerLog, + }); + + final result = await SyncService( + stateStore: InMemorySyncStateStore(), + ).sync(repo, remote); + + expect(result.mergedDevices, 1); + expect(remote.reads, contains('todo-sync/notes/phone.json')); + }); + }); +} diff --git a/test/sync_service_test.dart b/test/sync_service_test.dart index 81ab3b1..9b326fa 100644 --- a/test/sync_service_test.dart +++ b/test/sync_service_test.dart @@ -8,72 +8,27 @@ import 'package:todo/data/note.dart'; import 'package:todo/data/note_repository.dart'; import 'package:todo/sync/sync_service.dart'; -http.Response _response(int statusCode, [Object? jsonBody]) => - http.Response(jsonEncode(jsonBody ?? {}), statusCode); - -http.Response _fileWith(String text, {String? sha}) => - _response(200, {'content': base64.encode(utf8.encode(text)), 'sha': ?sha}); - -class _PutCall { - _PutCall(this.path, this.body); - final String path; - final Map body; -} - -/// Mock GitHub Contents API router matching crdt_sync's client: GET -/// `.../contents/` returns [contentResponses]`[key]` (404 if absent), the -/// bare repo-existence GET always succeeds, and every PUT is recorded. -({http.Client httpClient, List<_PutCall> putCalls}) _mockGitHub({ - Map contentResponses = const {}, -}) { - final putCalls = <_PutCall>[]; - final client = http_testing.MockClient((request) async { - final path = request.url.path; - if (!path.contains('/contents/')) return _response(200); // repo-exists - final key = path.split('/contents/').last; - if (request.method == 'PUT') { - putCalls.add( - _PutCall(key, jsonDecode(request.body) as Map), - ); - return _response(200); - } - return contentResponses[key] ?? _response(404); - }); - return (httpClient: client, putCalls: putCalls); -} - -GitHubClient _github(http.Client mock) => - GitHubClient(owner: 'o', repo: 'r', token: 't', httpClient: mock); - -Note _note(String id, String text) => Note( - id: id, - text: text, - priority: Priority.medium, - status: Status.todo, - createdAt: DateTime(2026, 6, 15), - updatedAt: DateTime(2026, 6, 15), -); +import 'sync_service_fakes.dart'; void main() { - _revisionTrackingTests(); test('pulls and merges another device, then pushes its own', () async { final other = await NoteRepository.openInMemory(nodeId: 'otherNode'); - await other.upsert(_note('x', 'from other device')); + await other.upsert(note('x', 'from other device')); final otherLog = logToJson(other.exportLog()); await other.close(); - final (:httpClient, :putCalls) = _mockGitHub( + final (:httpClient, :putCalls) = mockGitHub( contentResponses: { - 'todo-sync/notes': _response(200, [ + 'todo-sync/notes': response(200, [ {'name': 'otherNode.json'}, ]), - 'todo-sync/notes/otherNode.json': _fileWith(otherLog), + 'todo-sync/notes/otherNode.json': fileWith(otherLog), }, ); final local = await NoteRepository.openInMemory(nodeId: 'localNode'); addTearDown(local.close); - final result = await const SyncService().sync(local, _github(httpClient)); + final result = await const SyncService().sync(local, github(httpClient)); expect(result.mergedDevices, 1); expect(result.pushed, isTrue); @@ -105,49 +60,49 @@ void main() { }); test('with no remote files, still pushes its own log', () async { - final (:httpClient, :putCalls) = _mockGitHub( - contentResponses: {'todo-sync/notes': _response(200, [])}, + final (:httpClient, :putCalls) = mockGitHub( + contentResponses: {'todo-sync/notes': response(200, [])}, ); final local = await NoteRepository.openInMemory(nodeId: 'localNode'); addTearDown(local.close); - final result = await const SyncService().sync(local, _github(httpClient)); + final result = await const SyncService().sync(local, github(httpClient)); expect(result.mergedDevices, 0); expect(putCalls, hasLength(1)); }); test('updates its own already-present file using its resolved sha', () async { - final (:httpClient, :putCalls) = _mockGitHub( + final (:httpClient, :putCalls) = mockGitHub( contentResponses: { - 'todo-sync/notes': _response(200, [ + 'todo-sync/notes': response(200, [ {'name': 'localNode.json'}, ]), - 'todo-sync/notes/localNode.json': _fileWith('{}', sha: 'own-sha-123'), + 'todo-sync/notes/localNode.json': fileWith('{}', sha: 'own-sha-123'), }, ); final local = await NoteRepository.openInMemory(nodeId: 'localNode'); addTearDown(local.close); - final result = await const SyncService().sync(local, _github(httpClient)); + final result = await const SyncService().sync(local, github(httpClient)); expect(result.mergedDevices, 0); // own file is not a peer to merge expect(putCalls.single.body['sha'], 'own-sha-123'); // put reused the sha }); test('a corrupt or wrong-shape peer file is skipped, not fatal', () async { - final (:httpClient, :putCalls) = _mockGitHub( + final (:httpClient, :putCalls) = mockGitHub( contentResponses: { - 'todo-sync/notes': _response(200, [ + 'todo-sync/notes': response(200, [ {'name': 'bad1.json'}, {'name': 'bad2.json'}, ]), - 'todo-sync/notes/bad1.json': _fileWith('{not json'), - 'todo-sync/notes/bad2.json': _fileWith('[]'), + 'todo-sync/notes/bad1.json': fileWith('{not json'), + 'todo-sync/notes/bad2.json': fileWith('[]'), }, ); final local = await NoteRepository.openInMemory(nodeId: 'localNode'); addTearDown(local.close); - final result = await const SyncService().sync(local, _github(httpClient)); + final result = await const SyncService().sync(local, github(httpClient)); expect(result.mergedDevices, 0); // both bad files skipped expect(putCalls, hasLength(1)); // The skips are reported, not silent: that's how a peer device dropping @@ -159,9 +114,9 @@ void main() { test('a listed-but-unfetchable peer file is reported as skipped', () async { // The file appears in the directory listing but its GET 404s (deleted // between list and fetch, or contents unavailable). - final (:httpClient, :putCalls) = _mockGitHub( + final (:httpClient, :putCalls) = mockGitHub( contentResponses: { - 'todo-sync/notes': _response(200, [ + 'todo-sync/notes': response(200, [ {'name': 'gone.json'}, ]), }, @@ -169,188 +124,9 @@ void main() { final local = await NoteRepository.openInMemory(nodeId: 'localNode'); addTearDown(local.close); - final result = await const SyncService().sync(local, _github(httpClient)); + final result = await const SyncService().sync(local, github(httpClient)); expect(result.mergedDevices, 0); expect(result.skippedFiles, ['gone.json']); expect(putCalls, hasLength(1)); // the push still happened }); } - -/// An in-memory [RemoteStore] that also serves revision maps in one read, -/// standing in for Firebase. Counts reads so tests can assert on *traffic* -/// rather than on the merge result. -class _FakeRemote implements RemoteStore, BulkMapReader { - _FakeRemote(this.files); - - final Map files; - final List reads = []; - final List writes = []; - - @override - Future> listDirectory(String path) async => files.keys - .where((k) => k.startsWith('$path/')) - .map((k) => k.substring(path.length + 1).split('/').first) - .toSet() - .toList(); - - @override - Future getFileText(String path) async { - reads.add(path); - return files[path]; - } - - @override - Future putFileText( - String path, - String text, { - required String message, - }) async { - writes.add(path); - files[path] = text; - } - - @override - Future> getStringMap(String path) async => { - for (final e in files.entries) - if (e.key.startsWith('$path/')) e.key.substring(path.length + 1): e.value, - }; - - @override - Future deleteFile(String path, {String message = ''}) async => - files.remove(path); - - @override - Future canAccessRemote() async => true; - - @override - void close() {} -} - -/// A [RemoteStore] with no bulk-map read, standing in for GitHub. -/// -/// Delegates rather than extends: a subclass of [_FakeRemote] would inherit -/// `BulkMapReader`, the very capability this fake exists to lack. -class _FakeRemoteWithoutBulkRead implements RemoteStore { - _FakeRemoteWithoutBulkRead(Map files) - : _inner = _FakeRemote(files); - - final _FakeRemote _inner; - - List get reads => _inner.reads; - - @override - Future> listDirectory(String path) => _inner.listDirectory(path); - - @override - Future getFileText(String path) => _inner.getFileText(path); - - @override - Future putFileText( - String path, - String text, { - required String message, - }) => _inner.putFileText(path, text, message: message); - - @override - Future deleteFile(String path, {String message = ''}) => - _inner.deleteFile(path, message: message); - - @override - Future canAccessRemote() => _inner.canAccessRemote(); - - @override - void close() => _inner.close(); -} - -void _revisionTrackingTests() { - group('revision tracking', () { - late NoteRepository repo; - - setUp(() async { - repo = await NoteRepository.openInMemory(nodeId: 'me'); - }); - - tearDown(() async => repo.close()); - - test('skips a peer whose revision has not changed', () async { - // The saving the free-tier budget depends on: an unchanged ~150 KB peer - // log must not be downloaded again. - final peerLog = logToJson(await _peerLog()); - final remote = _FakeRemote({ - 'todo-sync/notes/phone.json': peerLog, - 'todo-sync/revs/phone': revisionOf(peerLog), - }); - final store = InMemorySyncStateStore(); - - await SyncService(stateStore: store).sync(repo, remote); - remote.reads.clear(); - final second = await SyncService(stateStore: store).sync(repo, remote); - - expect(remote.reads, isNot(contains('todo-sync/notes/phone.json'))); - expect(second.skippedUnchanged, 1); - expect(second.mergedDevices, 0); - }); - - test('downloads again once the peer publishes a new revision', () async { - final first = logToJson(await _peerLog()); - final remote = _FakeRemote({ - 'todo-sync/notes/phone.json': first, - 'todo-sync/revs/phone': revisionOf(first), - }); - final store = InMemorySyncStateStore(); - await SyncService(stateStore: store).sync(repo, remote); - remote.reads.clear(); - - final changed = logToJson(await _peerLog(title: 'changed')); - remote.files['todo-sync/notes/phone.json'] = changed; - remote.files['todo-sync/revs/phone'] = revisionOf(changed); - final second = await SyncService(stateStore: store).sync(repo, remote); - - expect(remote.reads, contains('todo-sync/notes/phone.json')); - expect(second.mergedDevices, 1); - }); - - test('suppresses an unchanged push and publishes a revision', () async { - final remote = _FakeRemote({}); - final store = InMemorySyncStateStore(); - - final first = await SyncService(stateStore: store).sync(repo, remote); - expect(first.pushed, isTrue); - expect(remote.writes, [ - 'todo-sync/notes/me.json', - 'todo-sync/revs/me', - ]); - remote.writes.clear(); - - final second = await SyncService(stateStore: store).sync(repo, remote); - - expect(second.pushed, isFalse); - expect(remote.writes, isEmpty); - }); - - test('still syncs on a backend without a bulk-map read', () async { - // GitHub has no cheap revision map; correctness must not depend on the - // optimisation being available. - final peerLog = logToJson(await _peerLog()); - final remote = _FakeRemoteWithoutBulkRead({ - 'todo-sync/notes/phone.json': peerLog, - }); - - final result = await SyncService( - stateStore: InMemorySyncStateStore(), - ).sync(repo, remote); - - expect(result.mergedDevices, 1); - expect(remote.reads, contains('todo-sync/notes/phone.json')); - }); - }); -} - -/// Builds a one-note log as it would arrive from another device. -Future _peerLog({String title = 'from phone'}) async { - final peer = await NoteRepository.openInMemory(nodeId: 'phone'); - await peer.upsert(_note('peer-note', title)); - final log = peer.exportLog(); - await peer.close(); - return log; -}