Store buildNumber in presubmitJob on inProgress event#5008
Store buildNumber in presubmitJob on inProgress event#5008
buildNumber in presubmitJob on inProgress event#5008Conversation
|
🤖 Hi @ievdokdm, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
This pull request correctly adds the storage of the buildNumber in the presubmitJob record during the inProgress state. This ensures that the build number is available for monitoring or debugging purposes as soon as the build starts.
🔍 General Feedback
- The change is surgical and consistent with the existing logic for handling job completion.
- I've suggested adding a unit test to verify this behavior and prevent regressions in the future.
- The use of
state.buildNumberis safe because thePresubmitJob.buildNumbersetter correctly handles nullable integers.
| // completed. | ||
| } else if (state.status == TaskStatus.inProgress) { | ||
| presubmitJob.startTime = state.startTime!; | ||
| presubmitJob.buildNumber = state.buildNumber; |
There was a problem hiding this comment.
🟢 It would be beneficial to add a unit test to verify that the buildNumber is correctly updated when the task status is inProgress. This ensures that any future changes to this logic don't accidentally drop the build number at this stage.
For example, you could add a test case in app_dart/test/service/firestore/unified_check_run_test.dart:
test('updates check status and build number on inProgress', () async {
final state = const PresubmitJobState(
jobName: 'linux',
status: TaskStatus.inProgress,
attemptNumber: 1,
startTime: 2000,
buildNumber: 456,
);
final result = await UnifiedCheckRun.markConclusion(
firestoreService: firestoreService,
guardId: guardId,
state: state,
);
expect(result.result, PresubmitGuardConclusionResult.ok);
final checkDoc = await PresubmitJob.fromFirestore(
firestoreService,
PresubmitJobId(
slug: slug,
checkRunId: 123,
jobName: 'linux',
attemptNumber: 1,
),
);
expect(checkDoc.status, TaskStatus.inProgress);
expect(checkDoc.startTime, 2000);
expect(checkDoc.buildNumber, 456);
});There was a problem hiding this comment.
This is a good idea
Store
buildNumberinpresubmitJoboninProgresseventFix: flutter/flutter#184371