From 66330cf5d69c0100d4fe3eb719079be25c428d3a Mon Sep 17 00:00:00 2001 From: John McDole Date: Thu, 6 Aug 2026 16:09:21 -0700 Subject: [PATCH 1/3] feat(suppression): automatically unsuppress and re-suppress tests on GitHub issue events - Add IssueService and wire it to GithubWebhookSubscription to handle GitHub 'issues' webhook events. - Automatically unsuppress tests when an associated GitHub issue is closed. - Automatically re-suppress tests when an associated GitHub issue is reopened. - Add TestSuppression.canonicalizeIssueUrl to sanitize user input (stripping fragments, query params, subpaths, and trailing slashes) into canonical GitHub issue URLs on ingress. - Add unit and webhook integration tests for IssueService, TestSuppression, and UpdateSuppressedTest. --- app_dart/lib/cocoon_service.dart | 2 + app_dart/lib/server.dart | 7 +- .../src/model/firestore/suppressed_test.dart | 51 ++- .../github/webhook_subscription.dart | 24 ++ .../update_suppressed_test.dart | 26 +- app_dart/lib/src/service/issue_service.dart | 111 +++++++ .../lib/src/service/test_suppression.dart | 34 +- .../model/firestore/suppressed_test_test.dart | 21 ++ .../check_flaky_builders_test.dart | 1 - .../file_flaky_issue_and_pr_test.dart | 1 - .../github/webhook_subscription_test.dart | 93 +++++- .../update_suppressed_test_test.dart | 83 +++++ app_dart/test/service/issue_service_test.dart | 312 ++++++++++++++++++ .../test/service/issue_service_test_data.dart | 91 +++++ .../test/service/test_suppression_test.dart | 111 +++++++ cocoon.code-workspace | 9 +- .../lib/src/utilities/webhook_generators.dart | 81 +++++ 17 files changed, 1005 insertions(+), 53 deletions(-) create mode 100644 app_dart/lib/src/service/issue_service.dart create mode 100644 app_dart/test/service/issue_service_test.dart create mode 100644 app_dart/test/service/issue_service_test_data.dart create mode 100644 app_dart/test/service/test_suppression_test.dart diff --git a/app_dart/lib/cocoon_service.dart b/app_dart/lib/cocoon_service.dart index 6e2c346cbe..2fd0263f54 100644 --- a/app_dart/lib/cocoon_service.dart +++ b/app_dart/lib/cocoon_service.dart @@ -63,6 +63,8 @@ export 'src/service/flags/ordered_presubmit_flags.dart'; export 'src/service/flags/unified_check_run_flow_flags.dart'; export 'src/service/gerrit_service.dart'; export 'src/service/github_checks_service.dart'; +export 'src/service/issue_service.dart'; export 'src/service/luci_build_service.dart'; export 'src/service/pull_request_manager.dart'; export 'src/service/scheduler.dart'; +export 'src/service/test_suppression.dart'; diff --git a/app_dart/lib/server.dart b/app_dart/lib/server.dart index 4368ebc87f..bf90ba2230 100644 --- a/app_dart/lib/server.dart +++ b/app_dart/lib/server.dart @@ -27,7 +27,6 @@ import 'src/service/content_aware_hash_service.dart'; import 'src/service/discord_service.dart'; import 'src/service/log_analyzer.dart'; import 'src/service/scheduler/ci_yaml_fetcher.dart'; -import 'src/service/test_suppression.dart'; typedef Server = Future Function(Request); @@ -66,6 +65,11 @@ Server createServer({ cache: cache, ); + final issueService = IssueService( + firestore: firestore, + suppressionService: suppressionService, + ); + final handlers = { '/api/analyze-logs': AnalyzeLogs( config: config, @@ -110,6 +114,7 @@ Server createServer({ scheduler: scheduler, commitService: commitService, firestore: firestore, + issueService: issueService, ), '/api/v2/presubmit-luci-subscription': PresubmitLuciSubscription( cache: cache, diff --git a/app_dart/lib/src/model/firestore/suppressed_test.dart b/app_dart/lib/src/model/firestore/suppressed_test.dart index 076b6d39bb..83eb978378 100644 --- a/app_dart/lib/src/model/firestore/suppressed_test.dart +++ b/app_dart/lib/src/model/firestore/suppressed_test.dart @@ -64,9 +64,18 @@ final class SuppressedTest extends AppDocument { '$fieldRepository =': repository, '$fieldIsSuppressed =': true, }); - return docs.isEmpty - ? [] - : [for (final doc in docs) SuppressedTest.fromDocument(doc)]; + return [for (final doc in docs) SuppressedTest.fromDocument(doc)]; + } + + /// Returns all [SuppressedTest] documents matching [issueLink]. + static Future> getByIssueLink( + FirestoreService firestore, + String issueLink, + ) async { + final docs = await firestore.query(kCollectionId, { + '$fieldIssueLink =': issueLink, + }); + return [for (final doc in docs) SuppressedTest.fromDocument(doc)]; } /// Creates a new [SuppressedTest] document. @@ -93,20 +102,24 @@ final class SuppressedTest extends AppDocument { } /// The misbehaving test. - String get testName => fields[fieldName]!.stringValue!; + String get testName => fields[fieldName]?.stringValue ?? ''; /// The repository this test is evaluated with. - String get repository => fields[fieldRepository]!.stringValue!; + String get repository => fields[fieldRepository]?.stringValue ?? ''; /// A required github issue link describing why the test is suppressed. - String get issueLink => fields[fieldIssueLink]!.stringValue!; + String get issueLink => fields[fieldIssueLink]?.stringValue ?? ''; /// Whether this test is currently suppressed. - bool get isSuppressed => fields[fieldIsSuppressed]!.booleanValue!; + bool get isSuppressed => fields[fieldIsSuppressed]?.booleanValue ?? false; /// When this document was created. - DateTime get createTimestamp => - DateTime.parse(fields[fieldCreateTimestamp]!.timestampValue!); + DateTime get createTimestamp { + final timestamp = fields[fieldCreateTimestamp]?.timestampValue; + return timestamp != null + ? DateTime.parse(timestamp) + : DateTime.fromMillisecondsSinceEpoch(0); + } /// A list of updates to this document for audit purposes. List> get updates { @@ -114,18 +127,22 @@ final class SuppressedTest extends AppDocument { if (values == null) { return const []; } - return [...values.map(_valueToUpdateMap)]; + return [for (final value in values) _valueToUpdateMap(value)]; } static Map _valueToUpdateMap(Value value) { - final fields = value.mapValue!.fields!; + final fields = value.mapValue?.fields; + if (fields == null) { + return const {}; + } return { - updateFieldUser: fields[updateFieldUser]!.stringValue!, - updateFieldUpdateTimestamp: DateTime.parse( - fields[updateFieldUpdateTimestamp]!.timestampValue!, - ), - updateFieldNote: fields[updateFieldNote]!.stringValue!, - updateFieldAction: fields[updateFieldAction]!.stringValue!, + updateFieldUser: fields[updateFieldUser]?.stringValue ?? '', + updateFieldUpdateTimestamp: + fields[updateFieldUpdateTimestamp]?.timestampValue != null + ? DateTime.parse(fields[updateFieldUpdateTimestamp]!.timestampValue!) + : DateTime.fromMillisecondsSinceEpoch(0), + updateFieldNote: fields[updateFieldNote]?.stringValue ?? '', + updateFieldAction: fields[updateFieldAction]?.stringValue ?? '', }; } } diff --git a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart index 38ad5762c5..5491d2e26f 100644 --- a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart +++ b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart @@ -43,6 +43,7 @@ final class GithubWebhookSubscription extends SubscriptionHandler { required this.gerritService, required this.commitService, required this.firestore, + this.issueService, super.authProvider, this.pullRequestLabelProcessorProvider = PullRequestLabelProcessor.new, @visibleForTesting DateTime Function() now = DateTime.now, @@ -63,6 +64,7 @@ final class GithubWebhookSubscription extends SubscriptionHandler { final FirestoreService firestore; final PullRequestLabelProcessorProvider pullRequestLabelProcessorProvider; + final IssueService? issueService; @override Future post(Request request) async { @@ -78,6 +80,8 @@ final class GithubWebhookSubscription extends SubscriptionHandler { switch (webhook.event) { case 'pull_request': return _handlePullRequest(webhook.payload); + case 'issues': + return _handleIssues(webhook.payload); case 'merge_group': final result = await _handleMergeGroup( webhook.payload, @@ -417,6 +421,26 @@ final class GithubWebhookSubscription extends SubscriptionHandler { } } + Future _handleIssues(String rawRequest) async { + final issueEvent = _getIssueEvent(rawRequest); + if (issueEvent == null) { + throw const BadRequestException('Expected issue event.'); + } + if (issueService case final service?) { + await service.handleIssueEvent(issueEvent); + } + return Response.emptyOk; + } + + IssueEvent? _getIssueEvent(String request) { + try { + return IssueEvent.fromJson(jsonDecode(request) as Map); + } catch (e, s) { + log.warn('_getIssueEvent: Failed to parse $request', e, s); + return null; + } + } + LabeledEvent? _getLabeledEvent(String request) { try { return LabeledEvent.fromJson( diff --git a/app_dart/lib/src/request_handlers/update_suppressed_test.dart b/app_dart/lib/src/request_handlers/update_suppressed_test.dart index 34ef41466a..eb3be49d83 100644 --- a/app_dart/lib/src/request_handlers/update_suppressed_test.dart +++ b/app_dart/lib/src/request_handlers/update_suppressed_test.dart @@ -8,8 +8,6 @@ import 'package:github/github.dart'; import '../../cocoon_service.dart'; import '../request_handling/api_request_handler.dart'; import '../request_handling/exceptions.dart'; -import '../service/test_suppression.dart' - show SuppressingAction, TestSuppression; /// Manually updates the test suppression status. /// @@ -97,16 +95,17 @@ final class UpdateSuppressedTest extends ApiRequestHandler { 'Parameter "$_paramIssueLink" must be a string', ); } - issueLink = link; - // Validate issue link - final issueNumber = _parseIssueNumber(issueLink); - if (issueNumber == null) { + final canonicalUrl = TestSuppression.canonicalizeIssueUrl(link); + if (canonicalUrl == null) { throw const BadRequestException( 'Invalid issue link format, expected https://github.com/flutter/flutter/issues/1234', ); } + issueLink = canonicalUrl; + final issueNumber = int.parse(Uri.parse(canonicalUrl).pathSegments.last); + final githubService = await config.createGithubService(repository); final Issue? issue; try { @@ -146,19 +145,4 @@ final class UpdateSuppressedTest extends ApiRequestHandler { return Response.emptyOk; } - - int? _parseIssueNumber(String issueLink) { - // Expected format: https://github.com/flutter/flutter/issues/123456 - final uri = Uri.tryParse(issueLink); - if (uri == null || - uri.host != 'github.com' || - uri.pathSegments.length < 4 || - uri.pathSegments[uri.pathSegments.length - 2] != 'issues') { - return null; - } - - // Path segments: [flutter, flutter, issues, 123456] - // Or just check the last segment if it is a number - return int.tryParse(uri.pathSegments.last); - } } diff --git a/app_dart/lib/src/service/issue_service.dart b/app_dart/lib/src/service/issue_service.dart new file mode 100644 index 0000000000..88f8cf0715 --- /dev/null +++ b/app_dart/lib/src/service/issue_service.dart @@ -0,0 +1,111 @@ +// Copyright 2026 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:cocoon_server/logging.dart'; +import 'package:github/github.dart'; +import 'package:github/hooks.dart'; + +import '../model/firestore/suppressed_test.dart'; +import 'firestore.dart'; +import 'test_suppression.dart'; + +/// Processes GitHub issue events to unsuppress or re-suppress matching tests. +final class IssueService { + const IssueService({ + required this.firestore, + required this.suppressionService, + }); + + final FirestoreService firestore; + final TestSuppression suppressionService; + + /// Re-opens or closes suppressed tests matching a GitHub [IssueEvent]. + Future handleIssueEvent(IssueEvent event) async { + if (event.issue?.htmlUrl case final issueUrl? when issueUrl.isNotEmpty) { + switch (event.action) { + case 'closed': + await _handleIssueClosed(event, issueUrl); + case 'reopened': + await _handleIssueReopened(event, issueUrl); + case final action: + log.debug('Ignoring issue event action: $action'); + } + return; + } + log.debug('IssueEvent has no htmlUrl; skipping.'); + } + + Future _handleIssueClosed(IssueEvent event, String issueUrl) async { + final docs = await SuppressedTest.getByIssueLink(firestore, issueUrl); + final processed = {}; + + for (final doc in docs) { + // Tests can be suppressed at different times; we only need to handle + // this event once per unique test. + final key = '${doc.repository}/${doc.testName}'; + if (!processed.add(key)) { + continue; + } + + final latest = await SuppressedTest.getLatest( + firestore, + doc.repository, + doc.testName, + ); + if (latest == null || + latest.issueLink != issueUrl || + !latest.isSuppressed) { + continue; + } + + log.info( + 'Unsuppressing test ${doc.testName} in ${doc.repository} due to issue closed ($issueUrl)', + ); + await suppressionService.updateSuppression( + testName: doc.testName, + email: event.sender?.login ?? 'github-webhook', + repository: RepositorySlug.full(doc.repository), + action: SuppressingAction.unsuppress, + note: 'Automatic unsuppression: issue $issueUrl was closed.', + ); + } + } + + Future _handleIssueReopened(IssueEvent event, String issueUrl) async { + final docs = await SuppressedTest.getByIssueLink(firestore, issueUrl); + final processed = {}; + + for (final doc in docs) { + // Tests can be suppressed at different times; we only need to handle + // this event once per unique test. + final key = '${doc.repository}/${doc.testName}'; + if (!processed.add(key)) { + continue; + } + + final latest = await SuppressedTest.getLatest( + firestore, + doc.repository, + doc.testName, + ); + if (latest == null || + latest.issueLink != issueUrl || + latest.isSuppressed) { + continue; + } + + log.info( + 'Re-suppressing test ${doc.testName} in ${doc.repository} due to issue reopened ($issueUrl)', + ); + await suppressionService.updateSuppression( + testName: doc.testName, + email: event.sender?.login ?? 'github-webhook', + repository: RepositorySlug.full(doc.repository), + action: SuppressingAction.suppress, + issueLink: issueUrl, + note: 'Automatic re-suppression: issue $issueUrl was reopened.', + ); + } + } +} diff --git a/app_dart/lib/src/service/test_suppression.dart b/app_dart/lib/src/service/test_suppression.dart index 14502e2dee..92c02a1695 100644 --- a/app_dart/lib/src/service/test_suppression.dart +++ b/app_dart/lib/src/service/test_suppression.dart @@ -99,11 +99,14 @@ class TestSuppression { return; } + final canonicalLink = issueLink != null + ? canonicalizeIssueUrl(issueLink) ?? issueLink + : null; final newSuppression = SuppressedTest( name: testName, repository: repository.fullName, issueLink: - issueLink ?? + canonicalLink ?? 'BUG: You have found a bug! Please report to https://www.github.com/flutter/flutter/issues/new', isSuppressed: true, createTimestamp: now, @@ -124,6 +127,35 @@ class TestSuppression { ); } + /// Canonicalizes a GitHub issue URL into `https://github.com///issues/`. + /// + /// Strips fragment anchors (e.g. `#issuecomment-123456`), query parameters, subpaths, + /// and trailing slashes. Returns `null` if the URL cannot be parsed into a valid GitHub issue link. + static String? canonicalizeIssueUrl(String issueUrl) { + final trimmed = issueUrl.trim(); + if (Uri.tryParse(trimmed) case Uri( + :final host, + :final pathSegments, + ) when host == 'github.com') { + final segments = [ + for (final s in pathSegments) + if (s.isNotEmpty) s, + ]; + if (segments case [ + final owner, + final repo, + 'issues', + final issueNumber, + ..., + ]) { + if (int.tryParse(issueNumber) != null) { + return 'https://github.com/$owner/$repo/issues/$issueNumber'; + } + } + } + return null; + } + Future isTestSuppressed({ required String testName, required RepositorySlug repository, diff --git a/app_dart/test/model/firestore/suppressed_test_test.dart b/app_dart/test/model/firestore/suppressed_test_test.dart index 79f9ee786e..14ad0e07f8 100644 --- a/app_dart/test/model/firestore/suppressed_test_test.dart +++ b/app_dart/test/model/firestore/suppressed_test_test.dart @@ -56,5 +56,26 @@ void main() { DateTime.fromMillisecondsSinceEpoch(1234567890, isUtc: true), ); }); + + test('handles legacy document missing issueLink gracefully', () { + final doc = Document( + name: + 'projects/flutter-dashboard/databases/cocoon/documents/suppressed_tests/legacy_doc', + fields: { + 'name': 'my_test'.toValue(), + 'repository': 'flutter/flutter'.toValue(), + 'isSuppressed': true.toValue(), + 'createTimestamp': DateTime.fromMillisecondsSinceEpoch( + 1234567890, + ).toValue(), + }, + ); + + final suppressedTest = SuppressedTest.fromDocument(doc); + expect(suppressedTest.testName, 'my_test'); + expect(suppressedTest.repository, 'flutter/flutter'); + expect(suppressedTest.issueLink, ''); + expect(suppressedTest.isSuppressed, true); + }); }); } diff --git a/app_dart/test/request_handlers/check_flaky_builders_test.dart b/app_dart/test/request_handlers/check_flaky_builders_test.dart index 596f2bc8f0..dc9144ee9d 100644 --- a/app_dart/test/request_handlers/check_flaky_builders_test.dart +++ b/app_dart/test/request_handlers/check_flaky_builders_test.dart @@ -14,7 +14,6 @@ import 'package:cocoon_service/src/model/proto/internal/scheduler.pb.dart' import 'package:cocoon_service/src/request_handlers/flaky_handler_utils.dart'; import 'package:cocoon_service/src/service/big_query.dart'; import 'package:cocoon_service/src/service/github_service.dart'; -import 'package:cocoon_service/src/service/test_suppression.dart'; import 'package:github/github.dart'; import 'package:mockito/mockito.dart'; import 'package:test/test.dart'; diff --git a/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart b/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart index 022036fa11..895dd32e24 100644 --- a/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart +++ b/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart @@ -14,7 +14,6 @@ import 'package:cocoon_service/src/model/proto/internal/scheduler.pb.dart' import 'package:cocoon_service/src/request_handlers/flaky_handler_utils.dart'; import 'package:cocoon_service/src/service/big_query.dart'; import 'package:cocoon_service/src/service/github_service.dart'; -import 'package:cocoon_service/src/service/test_suppression.dart'; import 'package:collection/collection.dart'; import 'package:github/github.dart'; import 'package:mockito/mockito.dart'; diff --git a/app_dart/test/request_handlers/github/webhook_subscription_test.dart b/app_dart/test/request_handlers/github/webhook_subscription_test.dart index 43dad73bee..85f01d46f0 100644 --- a/app_dart/test/request_handlers/github/webhook_subscription_test.dart +++ b/app_dart/test/request_handlers/github/webhook_subscription_test.dart @@ -3,7 +3,6 @@ // found in the LICENSE file. import 'dart:async'; -import 'dart:io'; import 'package:buildbucket/buildbucket_pb.dart' as bbv2; import 'package:cocoon_common/core_extensions.dart'; @@ -12,21 +11,13 @@ import 'package:cocoon_integration_test/testing.dart'; import 'package:cocoon_server/logging.dart'; import 'package:cocoon_server_test/mocks.dart'; import 'package:cocoon_server_test/test_logging.dart'; -import 'package:cocoon_service/src/model/firestore/base.dart'; +import 'package:cocoon_service/cocoon_service.dart'; import 'package:cocoon_service/src/model/firestore/ci_staging.dart'; import 'package:cocoon_service/src/model/firestore/commit.dart' as fs; -import 'package:cocoon_service/src/model/firestore/pr_check_runs.dart'; import 'package:cocoon_service/src/model/github/checks.dart' hide CheckRun; -import 'package:cocoon_service/src/request_handlers/github/webhook_subscription.dart'; import 'package:cocoon_service/src/request_handling/exceptions.dart'; import 'package:cocoon_service/src/service/big_query.dart'; -import 'package:cocoon_service/src/service/cache_service.dart'; -import 'package:cocoon_service/src/service/config.dart'; -import 'package:cocoon_service/src/service/flags/dynamic_config.dart'; -import 'package:cocoon_service/src/service/flags/unified_check_run_flow_flags.dart'; import 'package:cocoon_service/src/service/github_service.dart'; -import 'package:cocoon_service/src/service/pull_request_manager.dart'; -import 'package:cocoon_service/src/service/scheduler.dart'; import 'package:fixnum/fixnum.dart'; import 'package:github/github.dart' hide Branch; import 'package:googleapis/bigquery/v2.dart'; @@ -72,6 +63,8 @@ void main() { const kReleaseHeadRef = 'cherrypicks-flutter-2.12-candidate.4'; late DateTime fakeNow; + late TestSuppression suppressionService; + late IssueService issueService; setUp(() { request = FakeHttpRequest(); @@ -170,6 +163,11 @@ void main() { fakeNow = DateTime.now(); cache = CacheService.inMemory(); + suppressionService = TestSuppression(firestore: firestore, cache: cache); + issueService = IssueService( + firestore: firestore, + suppressionService: suppressionService, + ); webhook = GithubWebhookSubscription( config: config, cache: cache, @@ -177,6 +175,7 @@ void main() { scheduler: scheduler, commitService: commitService, firestore: firestore, + issueService: issueService, pullRequestLabelProcessorProvider: ({ required Config config, @@ -3281,6 +3280,80 @@ void foo() { }); }); + group('github webhook issues event', () { + test('handles issues closed event to unsuppress tests', () async { + const issueLink = 'https://github.com/flutter/flutter/issues/999'; + final testDoc = SuppressedTest( + name: 'Linux fu', + repository: 'flutter/flutter', + issueLink: issueLink, + isSuppressed: true, + createTimestamp: DateTime.utc(2026, 1, 1), + ); + await firestore.createDocument( + testDoc, + collectionId: SuppressedTest.kCollectionId, + ); + + tester.message = generateIssueMessage( + action: 'closed', + htmlUrl: issueLink, + login: 'octocat', + ); + + await tester.post(webhook); + + final latest = await SuppressedTest.getLatest( + firestore, + 'flutter/flutter', + 'Linux fu', + ); + expect(latest, isNotNull); + expect(latest!.isSuppressed, isFalse); + expect(latest.updates.last['user'], 'octocat'); + expect( + latest.updates.last['note'], + contains('Automatic unsuppression: issue $issueLink was closed.'), + ); + }); + + test('handles issues reopened event to re-suppress tests', () async { + const issueLink = 'https://github.com/flutter/flutter/issues/999'; + final testDoc = SuppressedTest( + name: 'Linux fu', + repository: 'flutter/flutter', + issueLink: issueLink, + isSuppressed: false, + createTimestamp: DateTime.utc(2026, 1, 1), + ); + await firestore.createDocument( + testDoc, + collectionId: SuppressedTest.kCollectionId, + ); + + tester.message = generateIssueMessage( + action: 'reopened', + htmlUrl: issueLink, + login: 'octocat', + ); + + await tester.post(webhook); + + final latest = await SuppressedTest.getLatest( + firestore, + 'flutter/flutter', + 'Linux fu', + ); + expect(latest, isNotNull); + expect(latest!.isSuppressed, isTrue); + expect(latest.updates.last['user'], 'octocat'); + expect( + latest.updates.last['note'], + contains('Automatic re-suppression: issue $issueLink was reopened.'), + ); + }); + }); + group('github webhook merge_group event', () { setUpAll(() { Scheduler.debugCheckPretendDelay = Duration.zero; diff --git a/app_dart/test/request_handlers/update_suppressed_test_test.dart b/app_dart/test/request_handlers/update_suppressed_test_test.dart index 7ce020dd48..9de3baad2f 100644 --- a/app_dart/test/request_handlers/update_suppressed_test_test.dart +++ b/app_dart/test/request_handlers/update_suppressed_test_test.dart @@ -231,6 +231,89 @@ void main() { ); }); + test('creates new suppression when issueLink has trailing slash', () async { + githubService.issueResponse = Issue(state: 'open'); + + tester.request.body = jsonEncode({ + 'testName': 'my_test_trailing', + 'repository': 'flutter/flutter', + 'action': 'SUPPRESS', + 'issueLink': 'https://github.com/flutter/flutter/issues/123/', + 'note': 'Trailing slash test', + }); + + await tester.post(handler); + + expect( + firestore, + existsInStorage(SuppressedTest.metadata, [ + isSuppressedTest + .hasIssueLink('https://github.com/flutter/flutter/issues/123') + .hasTestName('my_test_trailing') + .hasRepository('flutter/flutter') + .hasIsSuppressed(isTrue), + ]), + ); + }); + + test( + 'creates new suppression and sanitizes issueLink with fragment anchor', + () async { + githubService.issueResponse = Issue(state: 'open'); + + tester.request.body = jsonEncode({ + 'testName': 'my_test_fragment', + 'repository': 'flutter/flutter', + 'action': 'SUPPRESS', + 'issueLink': + 'https://github.com/flutter/flutter/issues/123#issuecomment-98765', + 'note': 'Fragment anchor test', + }); + + await tester.post(handler); + + expect( + firestore, + existsInStorage(SuppressedTest.metadata, [ + isSuppressedTest + .hasIssueLink('https://github.com/flutter/flutter/issues/123') + .hasTestName('my_test_fragment') + .hasRepository('flutter/flutter') + .hasIsSuppressed(isTrue), + ]), + ); + }, + ); + + test( + 'creates new suppression and sanitizes issueLink with query parameters', + () async { + githubService.issueResponse = Issue(state: 'open'); + + tester.request.body = jsonEncode({ + 'testName': 'my_test_query', + 'repository': 'flutter/flutter', + 'action': 'SUPPRESS', + 'issueLink': + 'https://github.com/flutter/flutter/issues/123?notification_referrer_id=abc', + 'note': 'Query params test', + }); + + await tester.post(handler); + + expect( + firestore, + existsInStorage(SuppressedTest.metadata, [ + isSuppressedTest + .hasIssueLink('https://github.com/flutter/flutter/issues/123') + .hasTestName('my_test_query') + .hasRepository('flutter/flutter') + .hasIsSuppressed(isTrue), + ]), + ); + }, + ); + test('Creates a new record for new suppressions', () async { githubService.issueResponse = Issue(state: 'open'); diff --git a/app_dart/test/service/issue_service_test.dart b/app_dart/test/service/issue_service_test.dart new file mode 100644 index 0000000000..2d8e276ce1 --- /dev/null +++ b/app_dart/test/service/issue_service_test.dart @@ -0,0 +1,312 @@ +// Copyright 2026 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:convert'; + +import 'package:cocoon_integration_test/testing.dart'; +import 'package:cocoon_server_test/test_logging.dart'; +import 'package:cocoon_service/cocoon_service.dart'; +import 'package:github/github.dart'; +import 'package:github/hooks.dart'; +import 'package:googleapis/firestore/v1.dart'; +import 'package:test/test.dart'; + +import 'issue_service_test_data.dart'; + +void main() { + useTestLoggerPerTest(); + + late IssueService issueService; + late FakeFirestoreService firestore; + late CacheService cache; + late TestSuppression suppressionService; + + setUp(() { + firestore = FakeFirestoreService(); + cache = CacheService.inMemory(); + suppressionService = TestSuppression(firestore: firestore, cache: cache); + issueService = IssueService( + firestore: firestore, + suppressionService: suppressionService, + ); + }); + + group('IssueService', () { + test( + 'unsuppresses all matching tests when issue is closed (e.g. Linux fu and Linux bar)', + () async { + const issueLink = 'https://github.com/flutter/flutter/issues/188740'; + final testDoc1 = SuppressedTest( + name: 'Linux fu', + repository: 'flutter/flutter', + issueLink: issueLink, + isSuppressed: true, + createTimestamp: DateTime.utc(2026, 1, 1), + ); + final testDoc2 = SuppressedTest( + name: 'Linux bar', + repository: 'flutter/flutter', + issueLink: issueLink, + isSuppressed: true, + createTimestamp: DateTime.utc(2026, 1, 1), + ); + await firestore.createDocument( + testDoc1, + collectionId: SuppressedTest.kCollectionId, + ); + await firestore.createDocument( + testDoc2, + collectionId: SuppressedTest.kCollectionId, + ); + + final eventJson = issueEventJson( + action: 'closed', + htmlUrl: issueLink, + login: 'ievdokdm', + number: 188740, + ); + final event = IssueEvent.fromJson( + jsonDecode(eventJson) as Map, + ); + + await issueService.handleIssueEvent(event); + + final latest1 = await SuppressedTest.getLatest( + firestore, + 'flutter/flutter', + 'Linux fu', + ); + expect(latest1, isNotNull); + expect(latest1!.isSuppressed, isFalse); + expect(latest1.updates.last['user'], 'ievdokdm'); + expect( + latest1.updates.last['note'], + contains('Automatic unsuppression'), + ); + + final latest2 = await SuppressedTest.getLatest( + firestore, + 'flutter/flutter', + 'Linux bar', + ); + expect(latest2, isNotNull); + expect(latest2!.isSuppressed, isFalse); + expect(latest2.updates.last['user'], 'ievdokdm'); + expect( + latest2.updates.last['note'], + contains('Automatic unsuppression'), + ); + }, + ); + + test( + 're-suppresses all matching tests when issue is reopened (e.g. Linux fu and Linux bar)', + () async { + const issueLink = 'https://github.com/flutter/flutter/issues/188740'; + final testDoc1 = SuppressedTest( + name: 'Linux fu', + repository: 'flutter/flutter', + issueLink: issueLink, + isSuppressed: false, + createTimestamp: DateTime.utc(2026, 1, 1), + ); + final testDoc2 = SuppressedTest( + name: 'Linux bar', + repository: 'flutter/flutter', + issueLink: issueLink, + isSuppressed: false, + createTimestamp: DateTime.utc(2026, 1, 1), + ); + await firestore.createDocument( + testDoc1, + collectionId: SuppressedTest.kCollectionId, + ); + await firestore.createDocument( + testDoc2, + collectionId: SuppressedTest.kCollectionId, + ); + + final eventJson = issueEventJson( + action: 'reopened', + htmlUrl: issueLink, + login: 'ievdokdm', + number: 188740, + ); + final event = IssueEvent.fromJson( + jsonDecode(eventJson) as Map, + ); + + await issueService.handleIssueEvent(event); + + final latest1 = await SuppressedTest.getLatest( + firestore, + 'flutter/flutter', + 'Linux fu', + ); + expect(latest1, isNotNull); + expect(latest1!.isSuppressed, isTrue); + expect(latest1.updates.last['user'], 'ievdokdm'); + expect( + latest1.updates.last['note'], + contains('Automatic re-suppression'), + ); + + final latest2 = await SuppressedTest.getLatest( + firestore, + 'flutter/flutter', + 'Linux bar', + ); + expect(latest2, isNotNull); + expect(latest2!.isSuppressed, isTrue); + expect(latest2.updates.last['user'], 'ievdokdm'); + expect( + latest2.updates.last['note'], + contains('Automatic re-suppression'), + ); + }, + ); + + test( + 'does not unsuppress test if latest suppression has different issue link', + () async { + const oldIssueLink = 'https://github.com/flutter/flutter/issues/111'; + const newIssueLink = 'https://github.com/flutter/flutter/issues/222'; + final oldDoc = SuppressedTest( + name: 'Linux test', + repository: 'flutter/flutter', + issueLink: oldIssueLink, + isSuppressed: false, + createTimestamp: DateTime.utc(2026, 1, 1), + ); + final newDoc = SuppressedTest( + name: 'Linux test', + repository: 'flutter/flutter', + issueLink: newIssueLink, + isSuppressed: true, + createTimestamp: DateTime.utc(2026, 1, 2), + ); + await firestore.createDocument( + oldDoc, + collectionId: SuppressedTest.kCollectionId, + ); + await firestore.createDocument( + newDoc, + collectionId: SuppressedTest.kCollectionId, + ); + + // Event for the OLD issue being closed + final eventJson = issueEventJson( + action: 'closed', + htmlUrl: oldIssueLink, + number: 111, + ); + final event = IssueEvent.fromJson( + jsonDecode(eventJson) as Map, + ); + + await issueService.handleIssueEvent(event); + + final latest = await SuppressedTest.getLatest( + firestore, + 'flutter/flutter', + 'Linux test', + ); + expect(latest, isNotNull); + // Remains suppressed under newIssueLink + expect(latest!.isSuppressed, isTrue); + expect(latest.issueLink, newIssueLink); + }, + ); + + test('handles legacy documents missing issueLink without crashing', () async { + final legacyDoc = SuppressedTest.fromDocument( + Document( + name: + 'projects/flutter-dashboard/databases/cocoon/documents/suppressed_tests/legacy', + fields: { + 'name': 'Linux legacy'.toValue(), + 'repository': 'flutter/flutter'.toValue(), + 'isSuppressed': true.toValue(), + 'createTimestamp': DateTime.utc(2026, 1, 1).toValue(), + }, + ), + ); + await firestore.createDocument( + legacyDoc, + collectionId: SuppressedTest.kCollectionId, + ); + + final eventJson = issueEventJson( + action: 'closed', + htmlUrl: 'https://github.com/flutter/flutter/issues/12345', + number: 12345, + ); + final event = IssueEvent.fromJson( + jsonDecode(eventJson) as Map, + ); + + // Should complete without null assertion error + await issueService.handleIssueEvent(event); + }); + + test( + 'defaults sender to github-webhook when sender is missing in event', + () async { + const issueLink = 'https://github.com/flutter/flutter/issues/999'; + final testDoc = SuppressedTest( + name: 'Linux nosender', + repository: 'flutter/flutter', + issueLink: issueLink, + isSuppressed: true, + createTimestamp: DateTime.utc(2026, 1, 1), + ); + await firestore.createDocument( + testDoc, + collectionId: SuppressedTest.kCollectionId, + ); + + final event = IssueEvent( + action: 'closed', + issue: Issue(htmlUrl: issueLink, number: 999), + sender: null, + ); + + await issueService.handleIssueEvent(event); + + final latest = await SuppressedTest.getLatest( + firestore, + 'flutter/flutter', + 'Linux nosender', + ); + expect(latest, isNotNull); + expect(latest!.isSuppressed, isFalse); + expect(latest.updates.last['user'], 'github-webhook'); + }, + ); + + test( + 'ignores non-closed/non-reopened actions or missing htmlUrl', + () async { + final eventJson = issueEventJson( + action: 'labeled', + htmlUrl: 'https://github.com/flutter/flutter/issues/123', + number: 123, + ); + final event = IssueEvent.fromJson( + jsonDecode(eventJson) as Map, + ); + + await issueService.handleIssueEvent(event); + + final eventWithoutUrl = IssueEvent(action: 'closed', issue: Issue()); + + await issueService.handleIssueEvent(eventWithoutUrl); + + final eventWithoutIssue = IssueEvent(action: 'closed', issue: null); + + await issueService.handleIssueEvent(eventWithoutIssue); + }, + ); + }); +} diff --git a/app_dart/test/service/issue_service_test_data.dart b/app_dart/test/service/issue_service_test_data.dart new file mode 100644 index 0000000000..dc04fbc701 --- /dev/null +++ b/app_dart/test/service/issue_service_test_data.dart @@ -0,0 +1,91 @@ +// Copyright 2026 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +/// A realistic GitHub IssueEvent JSON payload based on issue #188740. +String issueEventJson({ + String action = 'closed', + String htmlUrl = 'https://github.com/flutter/flutter/issues/188740', + String login = 'codefu', + int number = 188740, + String title = 'Increase errors in Cocoon since Friday', +}) => + ''' +{ + "action": "$action", + "issue": { + "url": "https://api.github.com/repos/flutter/flutter/issues/$number", + "repository_url": "https://api.github.com/repos/flutter/flutter", + "html_url": "$htmlUrl", + "id": 4771077498, + "node_id": "I_kwDOAeUeuM8AAAABHGDdeg", + "number": $number, + "title": "$title", + "user": { + "login": "jtmcdole", + "id": 1924313, + "html_url": "https://github.com/jtmcdole", + "type": "User", + "site_admin": false + }, + "labels": [ + { + "id": 1578115393, + "name": "team-infra", + "color": "198022", + "default": false, + "description": "Owned by Infrastructure team" + }, + { + "id": 2096800592, + "name": "P1", + "color": "990000", + "default": false, + "description": "High-priority issues at the top of the work list" + } + ], + "state": "$action", + "locked": false, + "assignees": [ + { + "login": "$login", + "id": 6338570, + "html_url": "https://github.com/$login", + "type": "User" + } + ], + "assignee": { + "login": "$login", + "id": 6338570, + "html_url": "https://github.com/$login", + "type": "User" + }, + "created_at": "2026-06-29T19:40:37Z", + "updated_at": "2026-08-06T19:37:41Z", + "closed_at": "2026-08-06T19:37:41Z", + "author_association": "MEMBER", + "body": "Increase errors in Cocoon since Friday", + "state_reason": "completed" + }, + "repository": { + "id": 31792824, + "name": "flutter", + "full_name": "flutter/flutter", + "owner": { + "login": "flutter", + "id": 14101776, + "avatar_url": "https://avatars.githubusercontent.com/u/14101776?v=4", + "html_url": "https://github.com/flutter", + "type": "Organization" + }, + "html_url": "https://github.com/flutter/flutter" + }, + "sender": { + "login": "$login", + "id": 6338570, + "html_url": "https://github.com/$login", + "type": "User", + "site_admin": false + } +} +'''; diff --git a/app_dart/test/service/test_suppression_test.dart b/app_dart/test/service/test_suppression_test.dart new file mode 100644 index 0000000000..93439baed4 --- /dev/null +++ b/app_dart/test/service/test_suppression_test.dart @@ -0,0 +1,111 @@ +// Copyright 2026 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:cocoon_service/cocoon_service.dart'; +import 'package:test/test.dart'; + +void main() { + group('TestSuppression.canonicalizeIssueUrl', () { + test('canonicalizes standard GitHub issue URLs', () { + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter/issues/188740', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + }); + + test('strips trailing slashes', () { + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter/issues/188740/', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter/issues/188740///', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + }); + + test('strips fragment anchors (e.g. comment links)', () { + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter/issues/188740#issuecomment-1234567', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + }); + + test('strips query parameters', () { + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter/issues/188740?param=1&q=search', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + }); + + test('strips rest path segments (e.g. comments subpath)', () { + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter/issues/188740/comments', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter/issues/188740/comments/123/', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + }); + + test('handles combined trailing slashes, queries, and fragments', () { + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter/issues/188740/comments/?param=1#frag', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + }); + + test('trims surrounding whitespace', () { + expect( + TestSuppression.canonicalizeIssueUrl( + ' https://github.com/flutter/flutter/issues/188740 ', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + }); + + test('returns null for non-GitHub URLs or non-issue links', () { + expect( + TestSuppression.canonicalizeIssueUrl('https://example.com/foo/bar/'), + isNull, + ); + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter/pull/123/', + ), + isNull, + ); + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter', + ), + isNull, + ); + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://github.com/flutter/flutter/issues/notanumber', + ), + isNull, + ); + expect(TestSuppression.canonicalizeIssueUrl(' '), isNull); + }); + }); +} diff --git a/cocoon.code-workspace b/cocoon.code-workspace index b4b45992c4..7fcc29bf72 100644 --- a/cocoon.code-workspace +++ b/cocoon.code-workspace @@ -34,5 +34,12 @@ "path": "conductor" } ], - "settings": {} + "settings": { + "cSpell.words": [ + "unsuppress", + "unsuppresses", + "Unsuppressing", + "unsuppression" + ] + } } \ No newline at end of file diff --git a/packages/cocoon_integration_test/lib/src/utilities/webhook_generators.dart b/packages/cocoon_integration_test/lib/src/utilities/webhook_generators.dart index f6aaf9a160..402d3c8eb3 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/webhook_generators.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/webhook_generators.dart @@ -1352,3 +1352,84 @@ ${reason != null ? '"reason": "$reason",' : ''} } '''; } + +PushMessage generateIssueMessage({ + required String action, + String htmlUrl = 'https://github.com/flutter/flutter/issues/188740', + String login = 'ievdokdm', + int number = 188740, + String repository = 'flutter/flutter', +}) { + final payload = jsonEncode({ + 'action': action, + 'issue': { + 'url': 'https://api.github.com/repos/$repository/issues/$number', + 'repository_url': 'https://api.github.com/repos/$repository', + 'html_url': htmlUrl, + 'id': 4771077498, + 'node_id': 'I_kwDOAeUeuM8AAAABHGDdeg', + 'number': number, + 'title': 'Increase errors in Cocoon since Friday', + 'user': { + 'login': 'jtmcdole', + 'id': 1924313, + 'html_url': 'https://github.com/jtmcdole', + 'type': 'User', + 'site_admin': false, + }, + 'labels': [ + { + 'id': 1578115393, + 'name': 'team-infra', + 'color': '198022', + 'default': false, + }, + {'id': 2096800592, 'name': 'P1', 'color': '990000', 'default': false}, + ], + 'state': action, + 'locked': false, + 'assignees': [ + { + 'login': login, + 'id': 6338570, + 'html_url': 'https://github.com/$login', + 'type': 'User', + }, + ], + 'assignee': { + 'login': login, + 'id': 6338570, + 'html_url': 'https://github.com/$login', + 'type': 'User', + }, + 'created_at': '2026-06-29T19:40:37Z', + 'updated_at': '2026-08-06T19:37:41Z', + 'closed_at': '2026-08-06T19:37:41Z', + 'author_association': 'MEMBER', + 'body': 'Increase errors in Cocoon since Friday', + 'state_reason': 'completed', + }, + 'repository': { + 'id': 31792824, + 'name': repository.split('/').last, + 'full_name': repository, + 'owner': { + 'login': repository.split('/').first, + 'id': 14101776, + 'avatar_url': 'https://avatars.githubusercontent.com/u/14101776?v=4', + 'html_url': 'https://github.com/${repository.split('/').first}', + 'type': 'Organization', + }, + 'html_url': 'https://github.com/$repository', + }, + 'sender': { + 'login': login, + 'id': 6338570, + 'html_url': 'https://github.com/$login', + 'type': 'User', + 'site_admin': false, + }, + }); + final message = pb.GithubWebhookMessage(event: 'issues', payload: payload); + return PushMessage(data: message.writeToJson(), messageId: 'abc123'); +} From 93024cc5113da4085be0555bab1af4bba408c615 Mon Sep 17 00:00:00 2001 From: John McDole Date: Thu, 6 Aug 2026 16:31:25 -0700 Subject: [PATCH 2/3] fix: check for www.github.com and don't allow incorrect urls through at all --- .../lib/src/service/test_suppression.dart | 22 +++-- .../file_flaky_issue_and_pr_test.dart | 7 +- .../file_flaky_issue_and_pr_test_data.dart | 2 +- .../update_suppressed_test_test.dart | 31 +++---- .../checkrun_authentication_test.dart | 10 +- .../dashboard_authentication_test.dart | 10 +- .../test/service/test_suppression_test.dart | 92 +++++++++++++++++++ 7 files changed, 141 insertions(+), 33 deletions(-) diff --git a/app_dart/lib/src/service/test_suppression.dart b/app_dart/lib/src/service/test_suppression.dart index 92c02a1695..beccbae3c4 100644 --- a/app_dart/lib/src/service/test_suppression.dart +++ b/app_dart/lib/src/service/test_suppression.dart @@ -99,15 +99,23 @@ class TestSuppression { return; } - final canonicalLink = issueLink != null - ? canonicalizeIssueUrl(issueLink) ?? issueLink - : null; + if (issueLink == null) { + throw ArgumentError.notNull('issueLink'); + } + + final canonicalLink = canonicalizeIssueUrl(issueLink); + if (canonicalLink == null) { + throw ArgumentError.value( + issueLink, + 'issueLink', + 'Invalid GitHub issue URL format. Expected https://github.com///issues/', + ); + } + final newSuppression = SuppressedTest( name: testName, repository: repository.fullName, - issueLink: - canonicalLink ?? - 'BUG: You have found a bug! Please report to https://www.github.com/flutter/flutter/issues/new', + issueLink: canonicalLink, isSuppressed: true, createTimestamp: now, updates: [updateEntry], @@ -136,7 +144,7 @@ class TestSuppression { if (Uri.tryParse(trimmed) case Uri( :final host, :final pathSegments, - ) when host == 'github.com') { + ) when host == 'github.com' || host == 'www.github.com') { final segments = [ for (final s in pathSegments) if (s.isNotEmpty) s, diff --git a/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart b/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart index 895dd32e24..b45b2e4e34 100644 --- a/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart +++ b/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart @@ -100,7 +100,12 @@ void main() { ); }); // ignore: discarded_futures - when(mockIssuesService.create(any, any)).thenAnswer((_) async => Issue()); + when(mockIssuesService.create(any, any)).thenAnswer( + (_) async => Issue( + htmlUrl: 'https://github.com/flutter/flutter/issues/123', + number: 123, + ), + ); // when gets existing flaky issues. when( mockIssuesService.listByRepo( diff --git a/app_dart/test/request_handlers/file_flaky_issue_and_pr_test_data.dart b/app_dart/test/request_handlers/file_flaky_issue_and_pr_test_data.dart index a652e5ef5a..3e7e66499f 100644 --- a/app_dart/test/request_handlers/file_flaky_issue_and_pr_test_data.dart +++ b/app_dart/test/request_handlers/file_flaky_issue_and_pr_test_data.dart @@ -183,7 +183,7 @@ const String jobNotCompleteResponse = ''' '''; const String expectedSemanticsIntegrationTestNewIssueURL = - 'https://something.something'; + 'https://github.com/flutter/flutter/issues/123'; const String expectedSemanticsIntegrationTestTreeSha = 'abcdefg'; const int expectedSemanticsIntegrationTestPRNumber = 123; diff --git a/app_dart/test/request_handlers/update_suppressed_test_test.dart b/app_dart/test/request_handlers/update_suppressed_test_test.dart index 9de3baad2f..496300ab15 100644 --- a/app_dart/test/request_handlers/update_suppressed_test_test.dart +++ b/app_dart/test/request_handlers/update_suppressed_test_test.dart @@ -484,31 +484,22 @@ void main() { expect(isSuppressed, isFalse); }); - test('TestSuppression uses default bug link if issueLink is null', () async { + test('TestSuppression throws ArgumentError if issueLink is null', () async { final suppression = TestSuppression( firestore: firestore, cache: cache, now: () => fakeNow, ); - await suppression.updateSuppression( - testName: 'my_test', - email: 'test@example.com', - repository: RepositorySlug('flutter', 'flutter'), - action: SuppressingAction.suppress, - note: 'test note', - issueLink: null, - ); - - // Verify document created with default bug link - expect( - firestore, - existsInStorage(SuppressedTest.metadata, [ - isSuppressedTest - .hasIssueLink( - 'BUG: You have found a bug! Please report to https://www.github.com/flutter/flutter/issues/new', - ) - .hasTestName('my_test'), - ]), + await expectLater( + suppression.updateSuppression( + testName: 'my_test', + email: 'test@example.com', + repository: RepositorySlug('flutter', 'flutter'), + action: SuppressingAction.suppress, + note: 'test note', + issueLink: null, + ), + throwsA(isA()), ); }); } diff --git a/app_dart/test/request_handling/checkrun_authentication_test.dart b/app_dart/test/request_handling/checkrun_authentication_test.dart index dd2b45c94b..067588e0e7 100644 --- a/app_dart/test/request_handling/checkrun_authentication_test.dart +++ b/app_dart/test/request_handling/checkrun_authentication_test.dart @@ -61,7 +61,10 @@ void main() { final mockGitHub = MockGitHub(); when( - mockGitHub.getJSON('/user/$id', convert: anyNamed('convert')), + mockGitHub.getJSON( + '/user/$id', + convert: anyNamed('convert'), + ), ).thenAnswer((_) async => User(login: user)); when( @@ -105,7 +108,10 @@ void main() { final mockGitHub = MockGitHub(); when( - mockGitHub.getJSON('/user/$id', convert: anyNamed('convert')), + mockGitHub.getJSON( + '/user/$id', + convert: anyNamed('convert'), + ), ).thenAnswer((_) async => User(login: user)); when( diff --git a/app_dart/test/request_handling/dashboard_authentication_test.dart b/app_dart/test/request_handling/dashboard_authentication_test.dart index 56235cdb2d..b4a8505a1d 100644 --- a/app_dart/test/request_handling/dashboard_authentication_test.dart +++ b/app_dart/test/request_handling/dashboard_authentication_test.dart @@ -156,7 +156,10 @@ void main() { final mockGitHub = MockGitHub(); when( - mockGitHub.getJSON('/user/$id', convert: anyNamed('convert')), + mockGitHub.getJSON( + '/user/$id', + convert: anyNamed('convert'), + ), ).thenAnswer((_) async => User(login: user)); when( @@ -200,7 +203,10 @@ void main() { final mockGitHub = MockGitHub(); when( - mockGitHub.getJSON('/user/$id', convert: anyNamed('convert')), + mockGitHub.getJSON( + '/user/$id', + convert: anyNamed('convert'), + ), ).thenAnswer((_) async => User(login: user)); when( diff --git a/app_dart/test/service/test_suppression_test.dart b/app_dart/test/service/test_suppression_test.dart index 93439baed4..b1124ad56d 100644 --- a/app_dart/test/service/test_suppression_test.dart +++ b/app_dart/test/service/test_suppression_test.dart @@ -2,7 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'package:cocoon_integration_test/testing.dart'; import 'package:cocoon_service/cocoon_service.dart'; +import 'package:github/github.dart'; import 'package:test/test.dart'; void main() { @@ -82,6 +84,21 @@ void main() { ); }); + test('canonicalizes www.github.com issue URLs to github.com', () { + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://www.github.com/flutter/flutter/issues/188740', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + expect( + TestSuppression.canonicalizeIssueUrl( + 'https://www.github.com/flutter/flutter/issues/188740/#issuecomment-123', + ), + 'https://github.com/flutter/flutter/issues/188740', + ); + }); + test('returns null for non-GitHub URLs or non-issue links', () { expect( TestSuppression.canonicalizeIssueUrl('https://example.com/foo/bar/'), @@ -108,4 +125,79 @@ void main() { expect(TestSuppression.canonicalizeIssueUrl(' '), isNull); }); }); + + group('TestSuppression.updateSuppression validation', () { + late FakeFirestoreService firestore; + late CacheService cache; + late TestSuppression suppressionService; + + setUp(() { + firestore = FakeFirestoreService(); + cache = CacheService.inMemory(); + suppressionService = TestSuppression(firestore: firestore, cache: cache); + }); + + test('throws ArgumentError if issueLink is null on suppress', () async { + await expectLater( + suppressionService.updateSuppression( + testName: 'Linux test', + email: 'test@example.com', + repository: RepositorySlug('flutter', 'flutter'), + action: SuppressingAction.suppress, + note: 'Missing URL test', + issueLink: null, + ), + throwsA(isA()), + ); + }); + + test('does not throw if issueLink is null on unsuppress', () async { + await expectLater( + suppressionService.updateSuppression( + testName: 'Linux test', + email: 'test@example.com', + repository: RepositorySlug('flutter', 'flutter'), + action: SuppressingAction.unsuppress, + note: 'Unsuppress test', + ), + completes, + ); + }); + + test('throws ArgumentError if issueLink is invalid on suppress', () async { + await expectLater( + suppressionService.updateSuppression( + testName: 'Linux test', + email: 'test@example.com', + repository: RepositorySlug('flutter', 'flutter'), + action: SuppressingAction.suppress, + note: 'Invalid URL test', + issueLink: 'https://example.com/not-github', + ), + throwsA(isA()), + ); + }); + + test('canonicalizes www.github.com issueLink on suppress', () async { + await suppressionService.updateSuppression( + testName: 'Linux test', + email: 'test@example.com', + repository: RepositorySlug('flutter', 'flutter'), + action: SuppressingAction.suppress, + note: 'Canonical test', + issueLink: 'https://www.github.com/flutter/flutter/issues/188740/', + ); + + final latest = await SuppressedTest.getLatest( + firestore, + 'flutter/flutter', + 'Linux test', + ); + expect(latest, isNotNull); + expect( + latest!.issueLink, + 'https://github.com/flutter/flutter/issues/188740', + ); + }); + }); } From a75d43a36a89f3f93654200b4183427a9a621788 Mon Sep 17 00:00:00 2001 From: John McDole Date: Fri, 7 Aug 2026 09:05:43 -0700 Subject: [PATCH 3/3] call test logging --- app_dart/test/service/test_suppression_test.dart | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app_dart/test/service/test_suppression_test.dart b/app_dart/test/service/test_suppression_test.dart index b1124ad56d..5324a19f3a 100644 --- a/app_dart/test/service/test_suppression_test.dart +++ b/app_dart/test/service/test_suppression_test.dart @@ -3,11 +3,14 @@ // found in the LICENSE file. import 'package:cocoon_integration_test/testing.dart'; +import 'package:cocoon_server_test/test_logging.dart'; import 'package:cocoon_service/cocoon_service.dart'; import 'package:github/github.dart'; import 'package:test/test.dart'; void main() { + useTestLoggerPerTest(); + group('TestSuppression.canonicalizeIssueUrl', () { test('canonicalizes standard GitHub issue URLs', () { expect(