Skip to content

Commit 41b5aa6

Browse files
jtmcdolejtmcdole
authored
feat: do NOT skip presubmit optimizations for content hashing changes (#4858)
this isn't an engine change, but we do need to build and test engines because the hash just isn't there. this is a rare condition.
1 parent 312580b commit 41b5aa6

File tree

11 files changed

+147
-70
lines changed

11 files changed

+147
-70
lines changed

‎.github/workflows/app_dart_tests.yaml‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,11 @@ jobs:
1818
working-directory: app_dart
1919

2020
steps:
21-
- uses: dart-lang/setup-dart@v1
21+
- name: Set up Flutter
22+
uses: subosito/flutter-action@fd55f4c5af5b953cc57a2be44cb082c8f6635e8e
23+
with:
24+
channel: stable
25+
architecture: x64 # only needed for running locally (mac)
2226

2327
- name: Checkout code
2428
uses: actions/checkout@v5

‎.github/workflows/dashboard_tests.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ jobs:
2222
uses: subosito/flutter-action@fd55f4c5af5b953cc57a2be44cb082c8f6635e8e
2323
with:
2424
channel: stable
25+
architecture: x64 # only needed for running locally (mac)
2526

2627
- name: Checkout code
2728
uses: actions/checkout@v5

‎app_dart/lib/src/service/scheduler/files_changed_optimization.dart‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ final class FilesChangedOptimizer {
6868
case SuccessfulFilesChanged(:final filesChanged):
6969
var noSourceImpact = true;
7070
for (final file in filesChanged) {
71-
if (file == 'DEPS' || file.startsWith('engine/')) {
71+
if (_engineFilePaths.hasMatch(file)) {
7272
log.info(
7373
'$refusePrefix: Engine sources changed.\n${filesChanged.join('\n')}',
7474
);
@@ -102,6 +102,8 @@ final class FilesChangedOptimizer {
102102
'release-candidate-branch.version',
103103
);
104104

105+
static final _engineFilePaths = RegExp(r'^(DEPS|engine/.*|bin/internal/content_aware_hash\.(ps1|sh))$');
106+
105107
static final _configPaths = RegExp(p.posix.join(r'.(github|vscode)', r'.*'));
106108

107109
static bool _isWithinConfigDirectory(String path) {

‎app_dart/pubspec.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ dependencies:
3030
googleapis: 14.0.0
3131
googleapis_auth: 2.0.0
3232
gql: ^1.0.1-alpha+1730759315362
33-
graphql: ^5.2.1
33+
graphql: 5.2.1
3434
grpc: ^4.0.1
3535
http: ^1.2.1
3636
jose_plus: ^0.4.6

‎app_dart/test/service/scheduler/files_changed_optimization_test.dart‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,4 +289,77 @@ void main() {
289289
),
290290
);
291291
});
292+
293+
group('content_aware_hash', () {
294+
test('sh is considered an engine file for presubmits', () async {
295+
final optimizer = FilesChangedOptimizer(
296+
getFilesChanged: filesChanged(['bin/internal/content_aware_hash.sh']),
297+
ciYamlFetcher: ciYamlFetcher(slug: Config.flutterSlug),
298+
config: config(maxFilesChangedForSkippingEnginePhase: 100),
299+
);
300+
301+
await expectLater(
302+
optimizer.checkPullRequest(
303+
generatePullRequest(
304+
repo: 'flutter',
305+
changedFilesCount: 1,
306+
number: 123,
307+
),
308+
),
309+
completion(FilesChangedOptimization.none),
310+
);
311+
312+
expect(
313+
log,
314+
bufferedLoggerOf(
315+
contains(logThat(message: contains('Engine sources changed'))),
316+
),
317+
);
318+
});
319+
320+
test('ps1 is considered an engine file for presubmits', () async {
321+
final optimizer = FilesChangedOptimizer(
322+
getFilesChanged: filesChanged(['bin/internal/content_aware_hash.ps1']),
323+
ciYamlFetcher: ciYamlFetcher(slug: Config.flutterSlug),
324+
config: config(maxFilesChangedForSkippingEnginePhase: 100),
325+
);
326+
327+
await expectLater(
328+
optimizer.checkPullRequest(
329+
generatePullRequest(
330+
repo: 'flutter',
331+
changedFilesCount: 1,
332+
number: 123,
333+
),
334+
),
335+
completion(FilesChangedOptimization.none),
336+
);
337+
338+
expect(
339+
log,
340+
bufferedLoggerOf(
341+
contains(logThat(message: contains('Engine sources changed'))),
342+
),
343+
);
344+
});
345+
346+
test('others is not considered an engine file for presubmits', () async {
347+
final optimizer = FilesChangedOptimizer(
348+
getFilesChanged: filesChanged(['bin/internal/content_aware_hash.bar']),
349+
ciYamlFetcher: ciYamlFetcher(slug: Config.flutterSlug),
350+
config: config(maxFilesChangedForSkippingEnginePhase: 100),
351+
);
352+
353+
await expectLater(
354+
optimizer.checkPullRequest(
355+
generatePullRequest(
356+
repo: 'flutter',
357+
changedFilesCount: 1,
358+
number: 123,
359+
),
360+
),
361+
completion(FilesChangedOptimization.skipPresubmitEngine),
362+
);
363+
});
364+
});
292365
}

‎dashboard/lib/views/build_dashboard_page.dart‎

Lines changed: 56 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -136,58 +136,63 @@ class BuildDashboardPageState extends State<BuildDashboardPage> {
136136
child: Material(
137137
color: Colors.transparent,
138138
child: FocusTraversalGroup(
139-
child: Column(
140-
mainAxisSize: MainAxisSize.min,
141-
children: <Widget>[
142-
if (_smallScreen)
143-
..._buildRepositorySelectionWidgets(context, buildState),
144-
AnimatedBuilder(
145-
animation: buildState,
146-
builder: (context, child) {
147-
final isAuthenticated =
148-
buildState.authService.isAuthenticated;
149-
return TextButton(
150-
onPressed: isAuthenticated
151-
? buildState.refreshGitHubCommits
152-
: null,
153-
child: child!,
154-
);
155-
},
156-
child: const Text('Refresh GitHub Commits'),
157-
),
158-
Row(
159-
mainAxisSize: MainAxisSize.min,
160-
children: [Center(child: FilterPropertySheet(_filter))],
161-
),
162-
Row(
163-
mainAxisSize: MainAxisSize.min,
164-
mainAxisAlignment: MainAxisAlignment.center,
165-
children: <Widget>[
166-
TextButton(
167-
onPressed: _filter!.isDefault
168-
? null
169-
: () => _filter!.reset(),
170-
child: const Text('Defaults'),
171-
),
172-
TextButton(
173-
onPressed: _filter == _settingsBasis
174-
? null
175-
: () => _updateNavigation(context),
176-
child: const Text('Apply'),
177-
),
178-
TextButton(
179-
child: const Text('Cancel'),
180-
onPressed: () {
181-
if (_filter != _settingsBasis) {
182-
_filter!.reset();
183-
_filter!.applyMap(_settingsBasis!.toMap());
184-
}
185-
_removeSettingsDialog();
186-
},
139+
child: SingleChildScrollView(
140+
child: Column(
141+
mainAxisSize: MainAxisSize.min,
142+
children: <Widget>[
143+
if (_smallScreen)
144+
..._buildRepositorySelectionWidgets(context, buildState),
145+
AnimatedBuilder(
146+
animation: buildState,
147+
builder: (context, child) {
148+
final isAuthenticated =
149+
buildState.authService.isAuthenticated;
150+
return TextButton(
151+
onPressed: isAuthenticated
152+
? buildState.refreshGitHubCommits
153+
: null,
154+
child: child!,
155+
);
156+
},
157+
child: const Text('Refresh GitHub Commits'),
158+
),
159+
SingleChildScrollView(
160+
scrollDirection: Axis.horizontal,
161+
child: Row(
162+
mainAxisSize: MainAxisSize.min,
163+
children: [Center(child: FilterPropertySheet(_filter))],
187164
),
188-
],
189-
),
190-
],
165+
),
166+
Row(
167+
mainAxisSize: MainAxisSize.min,
168+
mainAxisAlignment: MainAxisAlignment.center,
169+
children: <Widget>[
170+
TextButton(
171+
onPressed: _filter!.isDefault
172+
? null
173+
: () => _filter!.reset(),
174+
child: const Text('Defaults'),
175+
),
176+
TextButton(
177+
onPressed: _filter == _settingsBasis
178+
? null
179+
: () => _updateNavigation(context),
180+
child: const Text('Apply'),
181+
),
182+
TextButton(
183+
child: const Text('Cancel'),
184+
onPressed: () {
185+
if (_filter != _settingsBasis) {
186+
_filter!.reset();
187+
_filter!.applyMap(_settingsBasis!.toMap());
188+
}
189+
_removeSettingsDialog();
190+
},
191+
),
192+
],
193+
),
194+
],
195+
),
191196
),
192197
),
193198
),

‎dashboard/lib/widgets/lattice.dart‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -76,11 +76,9 @@ class LatticeScrollView extends StatelessWidget {
7676
BuildContext context,
7777
ViewportOffset horizontalOffset,
7878
) => NotificationListener<OverscrollNotification>(
79-
onNotification:
80-
(notification) =>
81-
notification.metrics.axisDirection !=
82-
AxisDirection.right &&
83-
notification.metrics.axisDirection != AxisDirection.left,
79+
onNotification: (notification) =>
80+
notification.metrics.axisDirection != AxisDirection.right &&
81+
notification.metrics.axisDirection != AxisDirection.left,
8482
child: Scrollbar(
8583
thumbVisibility: true,
8684
controller: verticalController,
@@ -578,10 +576,9 @@ class _RenderLatticeBody extends RenderBox {
578576
super.attach(owner);
579577
_horizontalOffset.addListener(_handleOffsetChange);
580578
_verticalOffset.addListener(_handleOffsetChange);
581-
_tap =
582-
TapGestureRecognizer(debugOwner: this)
583-
..onTapDown = _handleTapDown
584-
..onTapUp = _handleTapUp;
579+
_tap = TapGestureRecognizer(debugOwner: this)
580+
..onTapDown = _handleTapDown
581+
..onTapUp = _handleTapUp;
585582
for (final child in _childrenByCoordinate.values) {
586583
child.attach(owner);
587584
}

‎dashboard/lib/widgets/user_sign_in.dart‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ class UserSignIn extends StatelessWidget {
3333
if (authService.user != null) {
3434
return PopupMenuButton<_SignInButtonAction>(
3535
offset: const Offset(0, 50),
36-
itemBuilder:
37-
(BuildContext context) => <PopupMenuEntry<_SignInButtonAction>>[
36+
itemBuilder: (BuildContext context) =>
37+
<PopupMenuEntry<_SignInButtonAction>>[
3838
const PopupMenuItem<_SignInButtonAction>(
3939
value: _SignInButtonAction.logout,
4040
child: Text('Log out'),

‎dashboard/test/goldens/build_dashboard.defaultPropertySheet.dark.png‎

1.61 KB
Loading

‎dashboard/test/goldens/build_dashboard.defaultPropertySheet.png‎

1.85 KB
Loading

0 commit comments

Comments
 (0)