From 8f3f4e266a25d8112d687cfbc1d41feeeebcaf59 Mon Sep 17 00:00:00 2001 From: Tom Brow Date: Thu, 12 Nov 2026 16:19:10 -0800 Subject: [PATCH] test(mobile): run push tests without a gateway dart-define (#9423) A plain `cd mobile && flutter test` failed 11 push and notification tests on a clean checkout of main, while CI passed. Agents running the full suite kept reporting them as "pre-existing, also fails on main", which hides real regressions in the same files. ### Root cause Push is gated on the compile-time `BUZZ_PUSH_GATEWAY_URL` define (`Env.pushGatewayConfigured`, since [#7261 configurable gateway](https://github.com/block/buzz/pull/7261) made the gateway optional). Without it the app is a push-less build: `readBuzzPushEndpointGrants()` returns no grants and `buzzPushLifecycleEnabled()` is false. CI's mobile shards, `just mobile-test` and the lefthook pre-push lane (which runs `just mobile-test`) all pass `--dart-define=BUZZ_PUSH_GATEWAY_URL=https://push.example`; a bare `flutter test` passes nothing. Each of the 11 was checked against that: - `notification_toggle_test` (5 cases) and `push_lease_revocation_outbox_test` "hosted relay assertion": no endpoint grants without a gateway (`Bad state: No element` at the grant lookup, `No endpoint grant exists for push lease tombstone`). - `push_bridge_test` "reads and exposes persisted endpoint grants" and "debug enrollment carries the configured relay and gateway URLs": the bridge returns `[]` before calling native (`Expected: an object with length of <1>, Actual: []`). - `push_bootstrap_test` "default push requires relay capability" and "new community starts registration": the lifecycle is disabled, so registration never starts (`Expected <1>, Actual <0>`). - `push_bridge_test` "development push gateway matches the compiled configuration": a stale test. It expected a `https://push.buzz.xyz` default that [#7261 configurable gateway](https://github.com/block/buzz/pull/7261) removed from the app; under CI's define both sides equal `push.example`, so it could never fail there. The same files pass on the same Mac with the define, so nothing is environmental. The `Unexpected native call takePendingNotificationResponse` log is unrelated noise: the toggle test's native mock didn't answer that call, which logs (and is caught) in CI too. ### Fix Tests get the gateway from a test hook instead of the define. `Env.pushGatewayUrl` becomes a getter over the compiled value with a `@visibleForTesting` `debugPushGatewayUrlOverride` (the same pattern as `debugUriStreamOverride`), and `test/flutter_test_config.dart` sets it for every test file. `push_unconfigured_build_test` sets it to `''`, so it runs in the main suite instead of being skipped there and run separately. CI, the Justfile, `scripts/flake-check.mjs` and the AGENTS/README check commands drop the define, so CI, `just mobile-test`, pre-push and a bare `flutter test` run the same thing. I chose this over documenting a canonical command because the define can't be made automatic: `flutter test` has no config file for defines, so any doc fix still leaves the obvious command red. A test hook makes every invocation agree. The stale test is replaced by one in the push-less file that binds the production read: with the override cleared, `Env.pushGatewayUrl` must equal the compiled `BUZZ_PUSH_GATEWAY_URL`. Now that CI compiles without the define, it fails if the app regains a built-in gateway default, which the old test never could. The toggle test's mock answers `takePendingNotificationResponse` with no pending response. What gets better: one test mode everywhere, and the push-less tests run in every run. Nothing gets worse: production reads the same compiled value (the override is never set outside tests, and an `aot-snapshot` probe shows AOT still folds the getter to the compiled constant and drops push-only code when the define is absent), and every test still runs in the push-capable mode CI used before. ### Validation - On a fresh origin/main worktree with the Hermit toolchain, a bare `flutter test` failed exactly these 11; the same files passed on the same machine with the define. - After the fix, the pre-push `just mobile-check` and `just mobile-test` (now a bare `flutter test` plus the recipe and secp256k1 checks) pass, with the push-less tests running instead of skipped. `scripts/flake-check.test.mjs` passes. - Falsifiability: re-adding a `defaultValue` to the compiled gateway fails the new no-default test, and removing `flutter_test_config.dart` brings back the other 10 failures. - Unrelated: an earlier full run also failed `community_provider_test` "discard orphan snapshots and image caches", which waits 500 ms for a background directory delete. The Mac was running other full suites at the time; the file passed 8 of 8 runs alone, with and without the define. It is not touched here. Signed-off-by: Tom Brow Co-authored-by: Tom Brow Co-authored-by: studio --- .github/workflows/_ci-clients.yml | 7 +------ AGENTS.md | 2 +- Justfile | 3 +-- mobile/README.md | 2 +- mobile/lib/shared/relay/relay_provider.dart | 16 ++++++++++++++-- .../settings/notification_toggle_test.dart | 4 +++- mobile/test/flutter_test_config.dart | 11 +++++++++++ mobile/test/shared/push/push_bridge_test.dart | 8 -------- .../push/push_unconfigured_build_test.dart | 16 +++++++++++++++- scripts/flake-check.mjs | 4 ---- scripts/flake-check.test.mjs | 10 ++-------- 11 files changed, 49 insertions(+), 34 deletions(-) create mode 100644 mobile/test/flutter_test_config.dart diff --git a/.github/workflows/_ci-clients.yml b/.github/workflows/_ci-clients.yml index 103708baa7..f067300e98 100644 --- a/.github/workflows/_ci-clients.yml +++ b/.github/workflows/_ci-clients.yml @@ -133,7 +133,7 @@ jobs: run: cd mobile && flutter analyze - name: Test if: matrix.lane == 'test' - run: /bin/bash scripts/mobile-test-shard.sh ${{ matrix.shard }} 3 --dart-define=BUZZ_PUSH_GATEWAY_URL=https://push.example + run: /bin/bash scripts/mobile-test-shard.sh ${{ matrix.shard }} 3 - name: Test native secp256k1 package if: matrix.lane == 'checks' run: cd mobile/packages/buzz_secp256k1 && dart pub get && dart test @@ -141,7 +141,6 @@ jobs: if: matrix.lane == 'checks' run: >- cd mobile && flutter test - --dart-define=BUZZ_PUSH_GATEWAY_URL=https://push.example --dart-define=BUZZ_DISTRIBUTION=internal --dart-define=BUZZ_BUILDERLAB_URL=https://id.example.org --dart-define=BUZZ_BUILDERLAB_RELAYS=buzz.example.org @@ -150,7 +149,6 @@ jobs: if: matrix.lane == 'checks' run: >- cd mobile && flutter test - --dart-define=BUZZ_PUSH_GATEWAY_URL=https://push.example --dart-define=BUZZ_DISTRIBUTION=public --dart-define=BUZZ_BUILDERLAB_URL=https://app.builderlab.xyz --dart-define=BUZZ_BUILDERLAB_RELAYS=communities.buzz.xyz @@ -170,9 +168,6 @@ jobs: test/features/pairing/pairing_hosted_handoff_test.dart test/shared/agents/remote_agents_test.dart test/features/agents - - name: Test mobile without push configuration - if: matrix.lane == 'checks' - run: cd mobile && flutter test test/shared/push/push_unconfigured_build_test.dart - name: Test mobile recipe argument forwarding if: matrix.lane == 'checks' run: /bin/bash scripts/test-mobile-gateway-recipes.sh diff --git a/AGENTS.md b/AGENTS.md index ab25292c9b..feb4104134 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -796,7 +796,7 @@ which buzz-app also uses), even where a payload is documented only in code. cd mobile dart format --output=none --set-exit-if-changed . flutter analyze -flutter test --dart-define=BUZZ_PUSH_GATEWAY_URL=https://push.example +flutter test ``` Or from repo root: `just mobile-fmt` (auto-fix), `just mobile-check` (lint + fmt check), `just mobile-test` (tests). diff --git a/Justfile b/Justfile index 60828f79cf..e3e00a1ef2 100644 --- a/Justfile +++ b/Justfile @@ -1070,8 +1070,7 @@ mobile-check: # Run mobile tests mobile-test: /bin/bash ./scripts/test-mobile-gateway-recipes.sh - unset GIT_DIR GIT_WORK_TREE; cd {{mobile_dir}} && flutter test --dart-define=BUZZ_PUSH_GATEWAY_URL=https://push.example - unset GIT_DIR GIT_WORK_TREE; cd {{mobile_dir}} && flutter test test/shared/push/push_unconfigured_build_test.dart + unset GIT_DIR GIT_WORK_TREE; cd {{mobile_dir}} && flutter test unset GIT_DIR GIT_WORK_TREE; cd {{mobile_dir}}/packages/buzz_secp256k1 && dart pub get && dart test # Regenerate the emoji dataset asset from desktop's emoji-mart install. diff --git a/mobile/README.md b/mobile/README.md index 5a27149bdd..ee0986865d 100644 --- a/mobile/README.md +++ b/mobile/README.md @@ -304,7 +304,7 @@ short sender pubkey, community subtitle, and no image. ```bash dart format --output=none --set-exit-if-changed . flutter analyze -flutter test --dart-define=BUZZ_PUSH_GATEWAY_URL=https://push.example +flutter test ``` Or from the repo root: `just mobile-check` and `just mobile-test`. diff --git a/mobile/lib/shared/relay/relay_provider.dart b/mobile/lib/shared/relay/relay_provider.dart index 19bde2cff3..692441d211 100644 --- a/mobile/lib/shared/relay/relay_provider.dart +++ b/mobile/lib/shared/relay/relay_provider.dart @@ -1,4 +1,5 @@ import 'package:buzz/shared/crypto/event_signing.dart'; +import 'package:flutter/foundation.dart' show visibleForTesting; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:nostr/nostr.dart' as nostr; @@ -79,10 +80,21 @@ class Env { ); /// Optional gateway origin. Without it this artifact has no push capability. - static const pushGatewayUrl = String.fromEnvironment('BUZZ_PUSH_GATEWAY_URL'); + static String get pushGatewayUrl => + debugPushGatewayUrlOverride ?? _compiledPushGatewayUrl; + + static const _compiledPushGatewayUrl = String.fromEnvironment( + 'BUZZ_PUSH_GATEWAY_URL', + ); + + /// Replaces the compiled gateway origin in tests, so a plain `flutter test` + /// covers both push-capable and push-less builds without a `--dart-define`. + /// `test/flutter_test_config.dart` sets it for every test file. + @visibleForTesting + static String? debugPushGatewayUrlOverride; /// Whether this artifact can offer push notification enrollment. - static const pushGatewayConfigured = pushGatewayUrl != ''; + static bool get pushGatewayConfigured => pushGatewayUrl != ''; } class RelayConfigNotifier extends Notifier { diff --git a/mobile/test/features/settings/notification_toggle_test.dart b/mobile/test/features/settings/notification_toggle_test.dart index b4078e75ca..96db70daa8 100644 --- a/mobile/test/features/settings/notification_toggle_test.dart +++ b/mobile/test/features/settings/notification_toggle_test.dart @@ -97,7 +97,9 @@ void main() { (call) async => switch (call.method) { 'enrollPush' => grant, 'endpointGrants' => [grant], - 'startRegistration' || 'syncPushSnapshot' => null, + 'startRegistration' || + 'syncPushSnapshot' || + 'takePendingNotificationResponse' => null, _ => throw StateError('Unexpected native call ${call.method}'), }, ); diff --git a/mobile/test/flutter_test_config.dart b/mobile/test/flutter_test_config.dart new file mode 100644 index 0000000000..90924d68b3 --- /dev/null +++ b/mobile/test/flutter_test_config.dart @@ -0,0 +1,11 @@ +import 'dart:async'; + +import 'package:buzz/shared/relay/relay_provider.dart'; + +/// Runs every test file as a push-capable build, so a plain `flutter test` +/// needs no `--dart-define=BUZZ_PUSH_GATEWAY_URL`. Tests of a build without a +/// gateway set [Env.debugPushGatewayUrlOverride] to `''` themselves. +Future testExecutable(FutureOr Function() testMain) async { + Env.debugPushGatewayUrlOverride = 'https://push.example'; + await testMain(); +} diff --git a/mobile/test/shared/push/push_bridge_test.dart b/mobile/test/shared/push/push_bridge_test.dart index b7a806d925..d59e8cbcac 100644 --- a/mobile/test/shared/push/push_bridge_test.dart +++ b/mobile/test/shared/push/push_bridge_test.dart @@ -230,14 +230,6 @@ void main() { }, ); - test('development push gateway matches the compiled configuration', () { - const expectedGateway = String.fromEnvironment( - 'BUZZ_PUSH_GATEWAY_URL', - defaultValue: 'https://push.buzz.xyz', - ); - expect(Env.pushGatewayUrl, expectedGateway); - }); - test('exposes APNs registration failure', () async { await TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger .handlePlatformMessage( diff --git a/mobile/test/shared/push/push_unconfigured_build_test.dart b/mobile/test/shared/push/push_unconfigured_build_test.dart index e621f41395..b2ca728ab8 100644 --- a/mobile/test/shared/push/push_unconfigured_build_test.dart +++ b/mobile/test/shared/push/push_unconfigured_build_test.dart @@ -18,6 +18,20 @@ import 'package:shared_preferences/shared_preferences.dart'; void main() { TestWidgetsFlutterBinding.ensureInitialized(); group('artifact without a gateway define', () { + setUp(() => Env.debugPushGatewayUrlOverride = ''); + tearDown(() => Env.debugPushGatewayUrlOverride = 'https://push.example'); + + test( + 'reads the gateway only from its define, with no built-in default', + () { + Env.debugPushGatewayUrlOverride = null; + expect( + Env.pushGatewayUrl, + const String.fromEnvironment('BUZZ_PUSH_GATEWAY_URL'), + ); + }, + ); + test( 'does not discover push capability or enter the opted-in lifecycle', () async { @@ -121,5 +135,5 @@ void main() { }, ); } - }, skip: Env.pushGatewayConfigured); + }); } diff --git a/scripts/flake-check.mjs b/scripts/flake-check.mjs index 13a0bbd53b..3c49edcf30 100644 --- a/scripts/flake-check.mjs +++ b/scripts/flake-check.mjs @@ -91,9 +91,6 @@ export const nodeTestArgs = (file) => "--test", ] : ["--import", "./test-loader.mjs", "--experimental-strip-types", "--test"]; -// Mirrors the mobile test shards in .github/workflows/_ci-clients.yml. -export const dartDefine = - "--dart-define=BUZZ_PUSH_GATEWAY_URL=https://push.example"; // Mirrors the postgres-ci default-filter in .config/nextest.toml // (`test(/postgres_tests::/)`, `binary(/^postgres_/)` and // `test(/(^|::)external_infra[^:]*::/)`): those tests need Postgres, Redis, @@ -1163,7 +1160,6 @@ function repeatDart(files, iterations) { "flutter", [ "test", - dartDefine, `--file-reporter=json:${reportFile}`, ...files.map((file) => file.replace(/^mobile\//, "")), ], diff --git a/scripts/flake-check.test.mjs b/scripts/flake-check.test.mjs index 304668d77a..48cd882035 100644 --- a/scripts/flake-check.test.mjs +++ b/scripts/flake-check.test.mjs @@ -17,7 +17,6 @@ import { chooseRust, classify, cratePackage, - dartDefine, hunkRanges, ITERATIONS, infraTest, @@ -1028,16 +1027,11 @@ test("reports give per-test statuses per iteration", () => { }); test("repeat invocations mirror the lanes they stand in for", () => { - // node:test flags come from desktop/package.json "test"; the Flutter define - // from the mobile test shards; nextest's flake profile must keep retries off. + // node:test flags come from desktop/package.json "test"; nextest's flake + // profile must keep retries off. const script = JSON.parse(read("../desktop/package.json")).scripts.test; for (const file of ["x.test.mjs", "x.jsdom-test.mjs"]) assert.ok(script.includes(`node ${nodeTestArgs(file).join(" ")} "`), file); - assert.ok( - read("../.github/workflows/_ci-clients.yml").includes( - `scripts/mobile-test-shard.sh \${{ matrix.shard }} 3 ${dartDefine}`, - ), - ); // nextest reads its config from the workspace root, so each Rust workspace // carries the flake profile. for (const config of [