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 242994029..ef72d0521 100644 --- a/app_dart/lib/src/request_handlers/update_suppressed_test.dart +++ b/app_dart/lib/src/request_handlers/update_suppressed_test.dart @@ -23,6 +23,7 @@ import '../service/firestore.dart'; /// - `repository`: The repository slug (e.g. `flutter/flutter`). /// - `action`: `SUPPRESS` or `UNSUPPRESS`. /// - `issueLink`: URL to the GitHub issue tracking the failure. +/// required if SUPPRESS /// - `note`: Optional note describing the action. final class UpdateSuppressedTest extends ApiRequestHandler { const UpdateSuppressedTest({ @@ -75,13 +76,6 @@ final class UpdateSuppressedTest extends ApiRequestHandler { ); } - final issueLink = body[_paramIssueLink]; - if (issueLink is! String) { - throw const BadRequestException( - 'Parameter "$_paramIssueLink" must be a string', - ); - } - final RepositorySlug repository; { final repositoryString = body[_paramRepository]; @@ -100,16 +94,26 @@ final class UpdateSuppressedTest extends ApiRequestHandler { ); } - // Validate issue link - final issueNumber = _parseIssueNumber(issueLink); - if (issueNumber == null) { - throw const BadRequestException( - 'Invalid issue link format, expected https://github.com/flutter/flutter/issues/1234', - ); - } + String? issueLink; // Validate issue exists and is open if suppressing if (action == _actionSuppress) { + final link = body[_paramIssueLink]; + if (link is! String) { + throw const BadRequestException( + 'Parameter "$_paramIssueLink" must be a string', + ); + } + issueLink = link; + + // Validate issue link + final issueNumber = _parseIssueNumber(issueLink); + if (issueNumber == null) { + throw const BadRequestException( + 'Invalid issue link format, expected https://github.com/flutter/flutter/issues/1234', + ); + } + final githubService = await config.createGithubService(repository); final issue = await githubService.getIssue( repository, @@ -160,8 +164,8 @@ final class UpdateSuppressedTest extends ApiRequestHandler { required String testName, required RepositorySlug repository, required String action, - required String issueLink, required String note, + String? issueLink, }) async { // Query for existing suppression - we assume there is at most one document // per (repo, name) based on business logic, though the DB constraint might @@ -197,7 +201,8 @@ final class UpdateSuppressedTest extends ApiRequestHandler { final updatedSuppression = SuppressedTest( name: testName, repository: repository.fullName, - issueLink: issueLink, + // issue: today we don't have UI affordance for updating the link. + issueLink: existingSuppression.issueLink, isSuppressed: isSuppressed, createTimestamp: existingSuppression.createTimestamp, updates: [...existingSuppression.updates, updateEntry], @@ -224,7 +229,7 @@ final class UpdateSuppressedTest extends ApiRequestHandler { final newSuppression = SuppressedTest( name: testName, repository: repository.fullName, - issueLink: issueLink, + issueLink: issueLink ?? 'BUG', isSuppressed: true, createTimestamp: now, updates: [updateEntry], diff --git a/app_dart/lib/src/request_handling/static_file_handler.dart b/app_dart/lib/src/request_handling/static_file_handler.dart index 45864e95c..8a612c862 100644 --- a/app_dart/lib/src/request_handling/static_file_handler.dart +++ b/app_dart/lib/src/request_handling/static_file_handler.dart @@ -39,9 +39,7 @@ final class StaticFileHandler extends RequestHandler { '.smcbin': 'application/octet-stream', }; - const mimeFileMap = { - 'apple-app-site-association': 'application/json', - }; + const mimeFileMap = {'apple-app-site-association': 'application/json'}; final resultPath = filePath == '/' ? '/index.html' : filePath; diff --git a/app_dart/test/model/firestore/suppressed_test_test.dart b/app_dart/test/model/firestore/suppressed_test_test.dart index 2c1fe2692..e64498c6e 100644 --- a/app_dart/test/model/firestore/suppressed_test_test.dart +++ b/app_dart/test/model/firestore/suppressed_test_test.dart @@ -8,7 +8,6 @@ import 'package:cocoon_service/src/model/firestore/suppressed_test.dart'; import 'package:googleapis/firestore/v1.dart'; import 'package:test/test.dart'; - void main() { useTestLoggerPerTest(); diff --git a/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart b/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart index 91e059f38..300bbaef7 100644 --- a/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart +++ b/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart @@ -60,8 +60,6 @@ void main() { ciYaml: examplePresubmitRescheduleFusionConfig, ); - - handler = PresubmitLuciSubscription( cache: CacheService(inMemory: true), config: config, 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 8e64fcc96..4dc9686c9 100644 --- a/app_dart/test/request_handlers/update_suppressed_test_test.dart +++ b/app_dart/test/request_handlers/update_suppressed_test_test.dart @@ -247,7 +247,7 @@ void main() { 'testName': 'my_test', 'repository': 'flutter/flutter', 'action': 'UNSUPPRESS', - 'issueLink': 'https://github.com/flutter/flutter/issues/123', + 'issueLink': 'https://github.com/flutter/flutter/issues/345', 'note': 'Closing issue', }); diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index af69a21af..713b27f37 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -3643,10 +3643,7 @@ targets: final check = PresubmitCompletedCheck.fromBuild(build, userData); - expect( - await scheduler.processCheckRunCompleted(check), - isTrue, - ); + expect(await scheduler.processCheckRunCompleted(check), isTrue); // Should schedule tests for the next stage (fusionTests) expect(fakeLuciBuildService.scheduledTryBuilds, isNotEmpty); @@ -3722,10 +3719,7 @@ targets: final check = PresubmitCompletedCheck.fromBuild(build, userData); - expect( - await scheduler.processCheckRunCompleted(check), - isTrue, - ); + expect(await scheduler.processCheckRunCompleted(check), isTrue); verify( mockGithubChecksUtil.updateCheckRun( @@ -3810,10 +3804,7 @@ targets: final check = PresubmitCompletedCheck.fromBuild(build, userData); - expect( - await scheduler.processCheckRunCompleted(check), - isTrue, - ); + expect(await scheduler.processCheckRunCompleted(check), isTrue); verify( mockGithubChecksUtil.updateCheckRun( diff --git a/dashboard/lib/service/appengine_cocoon.dart b/dashboard/lib/service/appengine_cocoon.dart index 5289e6f46..c3dfe8c06 100644 --- a/dashboard/lib/service/appengine_cocoon.dart +++ b/dashboard/lib/service/appengine_cocoon.dart @@ -264,12 +264,10 @@ class AppEngineCocoonService implements CocoonService { required String repo, required String testName, required bool suppress, - required String issueLink, + String? issueLink, String? note, }) async { - final updateTestSuppressionUrl = apiEndpoint( - '/api/update-suppressed-test', - ); + final updateTestSuppressionUrl = apiEndpoint('/api/update-suppressed-test'); final response = await _client.post( updateTestSuppressionUrl, headers: {'X-Flutter-IdToken': idToken}, @@ -277,7 +275,7 @@ class AppEngineCocoonService implements CocoonService { 'repository': repo, 'testName': testName, 'action': suppress ? 'SUPPRESS' : 'UNSUPPRESS', - 'issueLink': issueLink, + 'issueLink': ?issueLink, 'note': ?note, }), ); diff --git a/dashboard/lib/service/cocoon.dart b/dashboard/lib/service/cocoon.dart index c56ef06c1..6c02b834f 100644 --- a/dashboard/lib/service/cocoon.dart +++ b/dashboard/lib/service/cocoon.dart @@ -103,7 +103,7 @@ abstract class CocoonService { required String repo, required String testName, required bool suppress, - required String issueLink, + String? issueLink, String? note, }); } diff --git a/dashboard/lib/service/dev_cocoon.dart b/dashboard/lib/service/dev_cocoon.dart index 8d85cb69a..93454ceeb 100644 --- a/dashboard/lib/service/dev_cocoon.dart +++ b/dashboard/lib/service/dev_cocoon.dart @@ -146,7 +146,7 @@ class DevelopmentCocoonService implements CocoonService { required String repo, required String testName, required bool suppress, - required String issueLink, + String? issueLink, String? note, }) async { final list = _suppressedTests.putIfAbsent(repo, () => []); @@ -156,7 +156,8 @@ class DevelopmentCocoonService implements CocoonService { SuppressedTest( name: testName, repository: repo, - issueLink: issueLink, + issueLink: + issueLink ?? 'https://github.com/flutter/flutter/issues/123', createTimestamp: DateTime.now().millisecondsSinceEpoch, updates: [ SuppressionUpdate( diff --git a/dashboard/lib/state/build.dart b/dashboard/lib/state/build.dart index 2313ecb46..5cf9e04e4 100644 --- a/dashboard/lib/state/build.dart +++ b/dashboard/lib/state/build.dart @@ -426,7 +426,7 @@ class BuildState extends ChangeNotifier { Future updateTestSuppression({ required String testName, required bool suppress, - required String issueLink, + String? issueLink, String? note, }) async { if (!authService.isAuthenticated) { diff --git a/dashboard/lib/widgets/task_grid.dart b/dashboard/lib/widgets/task_grid.dart index 9c050c4e1..c64be8ee9 100644 --- a/dashboard/lib/widgets/task_grid.dart +++ b/dashboard/lib/widgets/task_grid.dart @@ -220,8 +220,8 @@ class _TaskGridState extends State { filter ??= TaskGridFilter(); // Pre-compute suppressed task names for faster lookup - final suppressedTaskNames = { - for (final s in taskGrid.buildState.suppressedTests) s.name, + final suppressedTasks = { + for (final s in taskGrid.buildState.suppressedTests) s.name: s, }; // 1: PREPARE ROWS @@ -274,7 +274,7 @@ class _TaskGridState extends State { // won't know how to sort the task later. scores.putIfAbsent(qualifiedTask, () => 0.0); } - final isSuppressed = suppressedTaskNames.contains(task.builderName); + final isSuppressed = suppressedTasks.containsKey(task.builderName); rows[commitCount - 1].cells[qualifiedTask] = LatticeCell( painter: _painterFor(task, isSuppressed), builder: _builderFor(task, isSuppressed), @@ -286,8 +286,8 @@ class _TaskGridState extends State { final tasks = scores.keys.toList() ..sort((QualifiedTask a, QualifiedTask b) { // Suppressed tests go first (far left) - final aSuppressed = suppressedTaskNames.contains(a.task); - final bSuppressed = suppressedTaskNames.contains(b.task); + final aSuppressed = suppressedTasks.containsKey(a.task); + final bSuppressed = suppressedTasks.containsKey(b.task); if (aSuppressed && !bSuppressed) { return -1; } @@ -307,43 +307,29 @@ class _TaskGridState extends State { [ const LatticeCell(), ...tasks.map((QualifiedTask task) { - final isSuppressed = suppressedTaskNames.contains(task.task); return LatticeCell( builder: (BuildContext context) { + final suppressedTest = suppressedTasks[task.task]; final icon = TaskIcon( qualifiedTask: task, - onTap: () => _showTestDetails(context, task), + onTap: (BuildContext context) => + _showTestDetails(context, task), ); - if (isSuppressed) { - final suppressedTest = taskGrid.buildState.suppressedTests - .firstWhere((s) => s.name == task.task); + if (suppressedTest != null) { // Format audit log - final tip = [ - 'Issue: ${suppressedTest.issueLink}', - for (var update - in [...suppressedTest.updates]..sort( - (a, b) => - b.updateTimestamp.compareTo(a.updateTimestamp), - )) - '[${DateTime.fromMillisecondsSinceEpoch(update.updateTimestamp)}] ${update.action} by ${update.user} ${update.note != null ? '- ${update.note}' : ''}', - ].join('\n'); - - return Tooltip( - message: tip, - child: Stack( - alignment: Alignment.bottomRight, - children: [ - Opacity(opacity: 0.3, child: icon), - const IgnorePointer( - child: Icon( - Icons.snooze, - fontWeight: FontWeight.bold, - size: 20, - color: Colors.red, - ), + return Stack( + alignment: Alignment.bottomRight, + children: [ + Opacity(opacity: 0.3, child: icon), + const IgnorePointer( + child: Icon( + Icons.snooze, + fontWeight: FontWeight.bold, + size: 20, + color: Colors.red, ), - ], - ), + ), + ], ); } return icon; @@ -510,6 +496,7 @@ class _TaskGridState extends State { final renderBox = context.findRenderObject() as RenderBox; final position = renderBox.localToGlobal(Offset.zero); + print('_showTestDetails: $position'); _taskOverlay = OverlayEntry( builder: (BuildContext context) => Stack( children: [ diff --git a/dashboard/lib/widgets/task_icon.dart b/dashboard/lib/widgets/task_icon.dart index 9fc9b807f..6ae16b2e5 100644 --- a/dashboard/lib/widgets/task_icon.dart +++ b/dashboard/lib/widgets/task_icon.dart @@ -22,7 +22,7 @@ class TaskIcon extends StatelessWidget { final QualifiedTask qualifiedTask; /// Callback when the icon is tapped. - final VoidCallback? onTap; + final void Function(BuildContext)? onTap; /// A lookup table for matching [stageName] to [Image]. /// @@ -91,7 +91,7 @@ class TaskIcon extends StatelessWidget { child: InkWell( onTap: () async { if (onTap != null) { - onTap!(); + onTap!(context); } else { await launchUrl(qualifiedTask.sourceConfigurationUrl); } diff --git a/dashboard/lib/widgets/test_details_popover.dart b/dashboard/lib/widgets/test_details_popover.dart index 3608d6027..bc1dcf9f3 100644 --- a/dashboard/lib/widgets/test_details_popover.dart +++ b/dashboard/lib/widgets/test_details_popover.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'package:cocoon_common/rpc_model.dart'; +import 'package:collection/collection.dart'; import 'package:flutter/material.dart'; import 'package:url_launcher/url_launcher.dart'; @@ -28,14 +30,17 @@ class TestDetailsPopover extends StatefulWidget { } class _TestDetailsPopoverState extends State { - bool get isSuppressed { - return widget.buildState.suppressedTests.any( + SuppressedTest? get suppressedTest { + return widget.buildState.suppressedTests.firstWhereOrNull( (s) => s.name == widget.qualifiedTask.task, ); } @override Widget build(BuildContext context) { + final suppressed = suppressedTest; + final isAuthenticated = widget.buildState.authService.isAuthenticated; + return Card( elevation: 4, child: Padding( @@ -45,60 +50,91 @@ class _TestDetailsPopoverState extends State { crossAxisAlignment: CrossAxisAlignment.start, mainAxisSize: MainAxisSize.min, children: [ - Text( + SelectableText( widget.qualifiedTask.task, style: Theme.of(context).textTheme.titleLarge, ), const SizedBox(height: 16), - AnimatedBuilder( - animation: widget.buildState, - builder: (context, child) { - final suppressed = isSuppressed; - final isAuthenticated = - widget.buildState.authService.isAuthenticated; + switch ((isAuthenticated, suppressed)) { + (false, _) => const Text( + 'Sign in to change tree blocking status', + style: TextStyle(fontStyle: FontStyle.italic), + ), + (true, null) => ElevatedButton.icon( + onPressed: () => _showSuppressDialog(context), + icon: const Icon(Icons.do_not_disturb_on_outlined), + label: const Text('Unblock Tree'), + style: ElevatedButton.styleFrom( + backgroundColor: Colors.amber.shade100, + foregroundColor: Colors.amber.shade900, + ), + ), + (true, _) => ElevatedButton.icon( + onPressed: () => _toggleSuppression(false), + icon: const Icon(Icons.check_circle_outline), + label: const Text('Remove Suppression'), + style: ElevatedButton.styleFrom( + backgroundColor: Colors.green.shade100, + foregroundColor: Colors.green.shade900, + ), + ), + }, + if (suppressed?.updates.isNotEmpty == true) ...[ + const SizedBox(height: 16), - if (!isAuthenticated) { - return const Text( - 'Sign in to change tree blocking status', - style: TextStyle(fontStyle: FontStyle.italic), - ); - } + Text( + 'Update History:', + style: Theme.of(context).textTheme.bodySmall?.copyWith( + height: 1.4, + fontWeight: FontWeight.bold, + ), + ), + const Divider(), + for (var update in suppressed!.updates.sortedBy( + (u) => u.updateTimestamp, + )) + Tooltip( + message: + '${DateTime.fromMillisecondsSinceEpoch(update.updateTimestamp)}', + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Text( + '${update.action} by ${update.user}', + style: Theme.of( + context, + ).textTheme.bodySmall?.copyWith(height: 1.4), + ), + if (update.note?.isNotEmpty == true) + Padding( + padding: const EdgeInsets.only(left: 8), + child: Text( + 'Note: ${update.note}', + style: Theme.of( + context, + ).textTheme.bodySmall?.copyWith(height: 1.4), + ), + ), + ], + ), + ), + + const Divider(), + ], - if (suppressed) { - // Test is suppressed (NOT Blocking Tree). - // Button: "Include Test" -> Unsuppress - return ElevatedButton.icon( - onPressed: () => _toggleSuppression( - false, - issueLink: - 'https://github.com/flutter/flutter/issues/123', - ), - icon: const Icon(Icons.check_circle_outline), - label: const Text('Include Test in Tree'), - style: ElevatedButton.styleFrom( - backgroundColor: Colors.green.shade100, - foregroundColor: Colors.green.shade900, - ), - ); - } else { - // Test is NOT suppressed (Blocking Tree). - // Button: "Unblock Tree" -> Suppress - return ElevatedButton.icon( - onPressed: () => _showSuppressDialog(context), - icon: const Icon(Icons.do_not_disturb_on_outlined), - label: const Text('Unblock Tree'), - style: ElevatedButton.styleFrom( - backgroundColor: Colors.amber.shade100, - foregroundColor: Colors.amber.shade900, - ), - ); - } - }, - ), const SizedBox(height: 16), - Row( - mainAxisAlignment: MainAxisAlignment.end, + Column( + crossAxisAlignment: CrossAxisAlignment.start, children: [ + if (suppressed != null) + TextButton.icon( + onPressed: () async { + await launchUrl(Uri.parse(suppressed.issueLink)); + }, + icon: const Icon(Icons.open_in_new), + label: const Text('Tracking Issue'), + ), + TextButton.icon( onPressed: () async { await launchUrl( @@ -197,7 +233,7 @@ class _TestDetailsPopoverState extends State { Future _toggleSuppression( bool suppress, { - required String issueLink, + String? issueLink, String? note, }) async { final success = await widget.buildState.updateTestSuppression( diff --git a/dashboard/test/utils/mocks.mocks.dart b/dashboard/test/utils/mocks.mocks.dart index 854a2c901..9c8b71f82 100644 --- a/dashboard/test/utils/mocks.mocks.dart +++ b/dashboard/test/utils/mocks.mocks.dart @@ -520,7 +520,7 @@ class MockCocoonService extends _i1.Mock implements _i3.CocoonService { required String? repo, required String? testName, required bool? suppress, - required String? issueLink, + String? issueLink, String? note, }) => (super.noSuchMethod( @@ -718,7 +718,7 @@ class MockBuildState extends _i1.Mock implements _i14.BuildState { _i8.Future updateTestSuppression({ required String? testName, required bool? suppress, - required String? issueLink, + String? issueLink, String? note, }) => (super.noSuchMethod( diff --git a/dashboard/test/widgets/test_details_popover_test.dart b/dashboard/test/widgets/test_details_popover_test.dart index c04a91e74..c34de67b6 100644 --- a/dashboard/test/widgets/test_details_popover_test.dart +++ b/dashboard/test/widgets/test_details_popover_test.dart @@ -68,7 +68,7 @@ void main() { ); expect(find.text('Unblock Tree'), findsOneWidget); - expect(find.text('Include Test in Tree'), findsNothing); + expect(find.text('Remove Suppression'), findsNothing); }); final suppressedTest = SuppressedTest( @@ -78,7 +78,7 @@ void main() { createTimestamp: 123, ); - testWidgets('shows Include Test in Tree button when suppressed', ( + testWidgets('shows `Remove Suppression` button when suppressed', ( WidgetTester tester, ) async { buildState.suppressedTests = [suppressedTest]; @@ -96,11 +96,11 @@ void main() { ), ); - expect(find.text('Include Test in Tree'), findsOneWidget); + expect(find.text('Remove Suppression'), findsOneWidget); expect(find.text('Unblock Tree'), findsNothing); }); - testWidgets('Include Test button unsuppresses test', ( + testWidgets('Remove Suppression button unsuppresses test', ( WidgetTester tester, ) async { buildState.suppressedTests = [suppressedTest]; @@ -118,7 +118,7 @@ void main() { ), ); - await tester.tap(find.text('Include Test in Tree')); + await tester.tap(find.text('Remove Suppression')); await tester.pump(); expect(buildState.updateTestSuppressionCalls, isNotEmpty); @@ -235,8 +235,8 @@ void main() { ), ); - // Tap Include Test - await tester.tap(find.text('Include Test in Tree')); + // Tap Remove Suppression + await tester.tap(find.text('Remove Suppression')); await tester.pump(); // Start async call await tester.pump(); // Resolve async call and show snackbar