Skip to content

Commit abb4b58

Browse files
ievdokdmievdokdm
authored
183602 Fix an issue with updating proper job attempt numbers on re-run (#4988)
### 1. Refactored Pull Request Retrieval - **Updated Handlers:** The logic in `rerun_all_failed_jobs.dart` and `rerun_failed_job.dart` has been updated to consistently use `PrCheckRuns.findPullRequestFor` to resolve the pull request via the commit SHA (`guard.commitSha`), rather than querying by PR number. - **Fixed Re-run Workflows:** Ensured that the handlers fetch the correct active pull request during re-runs and store the proper retry number/attempt information when invoking `reScheduleTryBuilds`. ### 2. Removed `pull_request_num` Field and Cache - **Model Cleanups:** The `pull_request_num` field was entirely removed from the `PrCheckRuns` Firestore model, including its related mappings and integration test matchers (`_pr_check_run.dart`). - **Removed Methods:** Deleted the obsolete `findPullRequestCachedForPullRequestNum` lookup method from both `PrCheckRuns` and the `Scheduler` service. Additionally, removed its related caching tests. ### 3. Service/Dependencies Cleanup - **Removed Unused Services:** Dropped the previously injected but unused `_githubService` from `scheduler.dart`. - **Code Formatting:** Ran standard Dart formatting rules (`dart format`) to ensure code uniformity in edited files. ### 4. Test Suite Adjustments - **Updated Mocks & Setup:** `rerun_all_failed_jobs_test.dart` and `rerun_failed_job_test.dart` were updated to properly seed data using `PrCheckRuns.initializeDocument(...)` mirroring production behavior. - **Updated Assertions:** Test assertions were updated to verify correct PR handling, most notably checking that `reScheduleTryBuilds` receives accurate target configurations (e.g., proper attempt increments like `containsPair(targetA, 2)`). - **Removed Deprecated Mocks:** Scrubbed generated files like `mocks.mocks.dart` and testing `data_seeder.dart` to remove references to the deleted fields and getters. fix: flutter/flutter#183602
1 parent e48778e commit abb4b58

12 files changed

Lines changed: 103 additions & 215 deletions

File tree

‎app_dart/lib/src/model/firestore/pr_check_runs.dart‎

Lines changed: 1 addition & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ import 'base.dart';
3838
final class PrCheckRuns extends AppDocument<PrCheckRuns> {
3939
static const kCollectionId = 'prCheckRuns';
4040
static const kPullRequestField = 'pull_request';
41-
static const kPullRequestNumField = 'pull_request_num';
4241
static const kSlugField = 'slug';
4342
static const kShaField = 'sha';
4443

@@ -78,14 +77,6 @@ final class PrCheckRuns extends AppDocument<PrCheckRuns> {
7877
fields[kPullRequestField] = json.encode(value.toJson()).toValue();
7978
}
8079

81-
/// The pull request number.
82-
int get pullRequestNum =>
83-
int.parse(fields[kPullRequestNumField]!.integerValue!);
84-
85-
set pullRequestNum(int value) {
86-
fields[kPullRequestNumField] = value.toValue();
87-
}
88-
8980
/// The head sha at the time this document was created for testing.
9081
String get sha => fields[kShaField]!.stringValue!;
9182

@@ -98,10 +89,7 @@ final class PrCheckRuns extends AppDocument<PrCheckRuns> {
9889
Map<String, String> get checkRuns {
9990
final checkRuns = <String, String>{};
10091
for (final MapEntry(:key, :value) in fields.entries) {
101-
if (key == kPullRequestField ||
102-
key == kPullRequestNumField ||
103-
key == kSlugField ||
104-
key == kShaField) {
92+
if (key == kPullRequestField || key == kSlugField || key == kShaField) {
10593
continue;
10694
}
10795
checkRuns[key] = value.stringValue!;
@@ -126,7 +114,6 @@ final class PrCheckRuns extends AppDocument<PrCheckRuns> {
126114

127115
final fields = <String, Value>{
128116
kPullRequestField: json.encode(pullRequest.toJson()).toValue(),
129-
kPullRequestNumField: pullRequest.number!.toValue(),
130117
kSlugField: Value(
131118
stringValue: json.encode(pullRequest.head!.repo!.slug().toJson()),
132119
),
@@ -202,7 +189,6 @@ final class PrCheckRuns extends AppDocument<PrCheckRuns> {
202189
return false;
203190
}
204191
prcr.pullRequest = pr;
205-
prcr.pullRequestNum = pr.number!;
206192
await firestore.commit(tx, documentsToWrites([prcr]));
207193
return true;
208194
}
@@ -215,23 +201,4 @@ final class PrCheckRuns extends AppDocument<PrCheckRuns> {
215201
final prcr = await _findPrCheckRunsForSha(firestoreService, sha);
216202
return prcr?.pullRequest;
217203
}
218-
219-
/// Retrieve the [PullRequest] for a given [pullRequestNum] or throw an error.
220-
static Future<PullRequest?> findPullRequestForPullRequestNum(
221-
FirestoreService firestoreService,
222-
int pullRequestNum,
223-
) async {
224-
final filterMap = <String, Object>{
225-
'$kPullRequestNumField =': pullRequestNum,
226-
};
227-
log.info(
228-
'findPullRequestForPullRequestNum($filterMap): finding prCheckRuns document',
229-
);
230-
final docs = await firestoreService.query(kCollectionId, filterMap);
231-
log.info('findPullRequestForPullRequestNum($filterMap): found: $docs');
232-
if (docs.isEmpty) {
233-
return null;
234-
}
235-
return PrCheckRuns.fromDocument(docs.first).pullRequest;
236-
}
237204
}

‎app_dart/lib/src/request_handlers/rerun_all_failed_jobs.dart‎

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import 'dart:async';
77
import 'package:github/github.dart';
88

99
import '../../cocoon_service.dart';
10+
import '../model/ci_yaml/target.dart';
1011
import '../request_handling/api_request_handler.dart';
1112
import '../request_handling/exceptions.dart';
1213
import '../service/firestore/unified_check_run.dart';
@@ -58,13 +59,17 @@ final class RerunAllFailedJobs extends ApiRequestHandler {
5859
throw NotFoundException('No PresubmitGuard found for PR $slug/$prNumber');
5960
}
6061

61-
final pullRequest = await _scheduler.findPullRequestCachedForPullRequestNum(
62-
slug,
63-
prNumber,
64-
);
65-
66-
if (pullRequest == null) {
67-
throw NotFoundException('No pull request found for PR $slug/$prNumber');
62+
final PullRequest pullRequest;
63+
try {
64+
pullRequest = await PrCheckRuns.findPullRequestFor(
65+
_firestore,
66+
guard.checkRunId,
67+
Config.kMergeQueueLockName,
68+
);
69+
} catch (e) {
70+
throw NotFoundException(
71+
'No pull request found for ${Config.kMergeQueueLockName} with ${guard.checkRunId} id',
72+
);
6873
}
6974

7075
final failedChecks = await UnifiedCheckRun.reInitializeFailedChecks(
@@ -83,20 +88,29 @@ final class RerunAllFailedJobs extends ApiRequestHandler {
8388
pullRequest,
8489
);
8590

86-
final failedTargets = targets
87-
.where((target) => failedChecks.checkRetries.containsKey(target.name))
88-
.toList();
91+
final checkRetries = <Target, int>{};
92+
for (final target in targets) {
93+
if (failedChecks.checkRetries.containsKey(target.name)) {
94+
checkRetries[target] = failedChecks.checkRetries[target.name]!;
95+
}
96+
}
97+
98+
if (checkRetries.length != failedChecks.checkRetries.length) {
99+
throw const NotFoundException(
100+
'Failed to find all failed targets in presubmit targets',
101+
);
102+
}
89103

90-
await _luciBuildService.scheduleTryBuilds(
91-
targets: failedTargets,
104+
await _luciBuildService.reScheduleTryBuilds(
105+
targets: checkRetries,
92106
pullRequest: pullRequest,
93107
engineArtifacts: artifacts,
94108
checkRunGuard: failedChecks.checkRunGuard,
95109
stage: failedChecks.stage,
96110
);
97111

98112
return Response.json({
99-
'results': failedTargets
113+
'results': checkRetries.keys
100114
.map((t) => {'builder': t.name, 'status': 'rescheduled'})
101115
.toList(),
102116
});

‎app_dart/lib/src/request_handlers/rerun_failed_job.dart‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,17 @@ final class RerunFailedJob extends ApiRequestHandler {
6161
throw NotFoundException('No PresubmitGuard found for PR $slug/$prNumber');
6262
}
6363

64-
final pullRequest = await _scheduler.findPullRequestCachedForPullRequestNum(
65-
slug,
66-
prNumber,
67-
);
68-
69-
if (pullRequest == null) {
70-
throw NotFoundException('No pull request found for PR $slug/$prNumber');
64+
final PullRequest pullRequest;
65+
try {
66+
pullRequest = await PrCheckRuns.findPullRequestFor(
67+
_firestore,
68+
guard.checkRunId,
69+
Config.kMergeQueueLockName,
70+
);
71+
} catch (e) {
72+
throw NotFoundException(
73+
'No pull request found for ${Config.kMergeQueueLockName} with ${guard.checkRunId} id',
74+
);
7175
}
7276

7377
final rerunInfo = await UnifiedCheckRun.reInitializeFailedJob(
@@ -95,8 +99,10 @@ final class RerunFailedJob extends ApiRequestHandler {
9599
throw BadRequestException('Target $buildName not found in .ci.yaml'),
96100
);
97101

98-
await _luciBuildService.scheduleTryBuilds(
99-
targets: [target],
102+
final retries = rerunInfo.checkRetries[buildName]!;
103+
104+
await _luciBuildService.reScheduleTryBuilds(
105+
targets: {target: retries},
100106
pullRequest: pullRequest,
101107
engineArtifacts: artifacts,
102108
checkRunGuard: rerunInfo.checkRunGuard,

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

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ class Scheduler {
7171
required BigQueryService bigQuery,
7272
}) : _luciBuildService = luciBuildService,
7373
_githubChecksService = githubChecksService,
74-
_githubService = githubService,
7574
_config = config,
7675
_getFilesChanged = getFilesChanged,
7776
_ciYamlFetcher = ciYamlFetcher,
@@ -87,7 +86,6 @@ class Scheduler {
8786
final GetFilesChanged _getFilesChanged;
8887
final Config _config;
8988
final GithubChecksService _githubChecksService;
90-
final GithubService _githubService;
9189
final CiYamlFetcher _ciYamlFetcher;
9290
final ContentAwareHashService _contentAwareHash;
9391
final LuciBuildService _luciBuildService;
@@ -1720,40 +1718,6 @@ $stacktrace
17201718
return pullRequest;
17211719
}
17221720

1723-
Future<PullRequest?> findPullRequestCachedForPullRequestNum(
1724-
RepositorySlug slug,
1725-
int pullRequestNum,
1726-
) async {
1727-
final logCrumb = 'findPullRequestCachedForPullRequestNum($pullRequestNum)';
1728-
PullRequest? pullRequest;
1729-
// Look up the PR in our cache first. This reduces github quota and requires less calls.
1730-
try {
1731-
pullRequest = await PrCheckRuns.findPullRequestForPullRequestNum(
1732-
_firestore,
1733-
pullRequestNum,
1734-
);
1735-
} catch (e, s) {
1736-
log.info('$logCrumb: unable to find PR in PrCheckRuns', e, s);
1737-
}
1738-
if (pullRequest == null) {
1739-
try {
1740-
pullRequest = await _githubService.getPullRequest(slug, pullRequestNum);
1741-
await PrCheckRuns.initializeDocument(
1742-
firestoreService: _firestore,
1743-
pullRequest: pullRequest,
1744-
checks: [],
1745-
);
1746-
} catch (e, s) {
1747-
log.warn('$logCrumb: unable to find PR in GitHub', e, s);
1748-
}
1749-
}
1750-
1751-
if (pullRequest == null) {
1752-
log.warn('$logCrumb: No pull request found');
1753-
}
1754-
return pullRequest;
1755-
}
1756-
17571721
Future<ProcessCheckRunResult> _reRunFailed(
17581722
cocoon_checks.CheckRunEvent checkRunEvent,
17591723
) async {

‎app_dart/test/model/firestore/pr_check_runs_test.dart‎

Lines changed: 3 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ void main() {
5050
json.encode(pr.toJson()),
5151
),
5252
)
53-
.hasPullRequestNum(pr.number)
5453
.hasSlug(pr.head!.repo!.slug())
5554
.hasSha(pr.head!.sha)
5655
.hasCheckRuns({'check 1': '1', 'check 2': '2'}),
@@ -84,33 +83,7 @@ void main() {
8483
expect(pr.id, 1234);
8584
});
8685

87-
test('query for pullRequestNum', () async {
88-
final pr = generatePullRequest(id: 1234, number: 5678);
89-
firestoreService.putDocument(
90-
Document(
91-
fields: {
92-
PrCheckRuns.kPullRequestField: Value(
93-
stringValue: json.encode(pr.toJson()),
94-
),
95-
PrCheckRuns.kPullRequestNumField: pr.number!.toValue(),
96-
},
97-
name: firestoreService.resolveDocumentName(
98-
PrCheckRuns.kCollectionId,
99-
'1234',
100-
),
101-
),
102-
);
103-
104-
final result = await PrCheckRuns.findPullRequestForPullRequestNum(
105-
firestoreService,
106-
5678,
107-
);
108-
109-
expect(result!.id, 1234);
110-
expect(result.number, 5678);
111-
});
112-
113-
test('updatePullRequestForSha updates pullRequestNum', () async {
86+
test('updatePullRequestForSha updates pullRequest', () async {
11487
final pr1 = generatePullRequest(id: 1234, number: 5678, headSha: 'abc');
11588
await PrCheckRuns.initializeDocument(
11689
firestoreService: firestoreService,
@@ -127,17 +100,11 @@ void main() {
127100

128101
expect(updated, isTrue);
129102

130-
final result = await PrCheckRuns.findPullRequestForPullRequestNum(
103+
final result = await PrCheckRuns.findPullRequestForSha(
131104
firestoreService,
132-
9999,
105+
'abc',
133106
);
134107
expect(result!.number, 9999);
135-
136-
final oldResult = await PrCheckRuns.findPullRequestForPullRequestNum(
137-
firestoreService,
138-
5678,
139-
);
140-
expect(oldResult, isNull);
141108
});
142109

143110
// Regression test for https://github.com/flutter/flutter/issues/166014.

‎app_dart/test/request_handlers/rerun_all_failed_jobs_test.dart‎

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,15 @@ void main() {
6262
);
6363
firestore.putDocument(guard);
6464

65-
final pullRequest = generatePullRequest();
65+
final pullRequest = generatePullRequest(headSha: guard.commitSha);
6666
scheduler.pullRequest = pullRequest;
67+
await PrCheckRuns.initializeDocument(
68+
firestoreService: firestore,
69+
pullRequest: pullRequest,
70+
checks: [
71+
generateCheckRun(guard.checkRunId, name: Config.kMergeQueueLockName),
72+
],
73+
);
6774

6875
final failedCheck = PresubmitCheck(
6976
slug: Config.flutterSlug,
@@ -84,7 +91,7 @@ void main() {
8491
);
8592

8693
when(
87-
mockLuciBuildService.scheduleTryBuilds(
94+
mockLuciBuildService.reScheduleTryBuilds(
8895
targets: anyNamed('targets'),
8996
pullRequest: anyNamed('pullRequest'),
9097
engineArtifacts: anyNamed('engineArtifacts'),
@@ -103,8 +110,8 @@ void main() {
103110
expect(response.statusCode, HttpStatus.ok);
104111

105112
verify(
106-
mockLuciBuildService.scheduleTryBuilds(
107-
targets: argThat(contains(targetA), named: 'targets'),
113+
mockLuciBuildService.reScheduleTryBuilds(
114+
targets: argThat(containsPair(targetA, 2), named: 'targets'),
108115
pullRequest: anyNamed('pullRequest'),
109116
engineArtifacts: anyNamed('engineArtifacts'),
110117
checkRunGuard: anyNamed('checkRunGuard'),
@@ -127,8 +134,15 @@ void main() {
127134
);
128135
firestore.putDocument(guard);
129136

130-
final pullRequest = generatePullRequest();
137+
final pullRequest = generatePullRequest(headSha: guard.commitSha);
131138
scheduler.pullRequest = pullRequest;
139+
await PrCheckRuns.initializeDocument(
140+
firestoreService: firestore,
141+
pullRequest: pullRequest,
142+
checks: [
143+
generateCheckRun(guard.checkRunId, name: Config.kMergeQueueLockName),
144+
],
145+
);
132146

133147
final failedCheck = PresubmitCheck(
134148
slug: Config.flutterSlug,
@@ -148,7 +162,7 @@ void main() {
148162
);
149163

150164
when(
151-
mockLuciBuildService.scheduleTryBuilds(
165+
mockLuciBuildService.reScheduleTryBuilds(
152166
targets: anyNamed('targets'),
153167
pullRequest: anyNamed('pullRequest'),
154168
engineArtifacts: anyNamed('engineArtifacts'),
@@ -171,8 +185,15 @@ void main() {
171185
);
172186
firestore.putDocument(guard);
173187

174-
final pullRequest = generatePullRequest();
188+
final pullRequest = generatePullRequest(headSha: guard.commitSha);
175189
scheduler.pullRequest = pullRequest;
190+
await PrCheckRuns.initializeDocument(
191+
firestoreService: firestore,
192+
pullRequest: pullRequest,
193+
checks: [
194+
generateCheckRun(guard.checkRunId, name: Config.kMergeQueueLockName),
195+
],
196+
);
176197

177198
tester.requestData = {
178199
'owner': 'flutter',
@@ -205,12 +226,6 @@ class TestScheduler extends FakeScheduler {
205226
int checkSuiteId,
206227
) async => pullRequest;
207228

208-
@override
209-
Future<PullRequest?> findPullRequestCachedForPullRequestNum(
210-
RepositorySlug slug,
211-
int pullRequestNum,
212-
) async => pullRequest;
213-
214229
List<Target>? targets;
215230
EngineArtifacts? engineArtifacts;
216231
@override

0 commit comments

Comments
 (0)