diff --git a/app_dart/lib/src/model/common/presubmit_check_state.dart b/app_dart/lib/src/model/common/presubmit_check_state.dart index a058e524d..13eeb2a50 100644 --- a/app_dart/lib/src/model/common/presubmit_check_state.dart +++ b/app_dart/lib/src/model/common/presubmit_check_state.dart @@ -19,6 +19,7 @@ class PresubmitCheckState { final int? startTime; final int? endTime; final String? summary; + final int? buildNumber; const PresubmitCheckState({ required this.buildName, @@ -27,6 +28,7 @@ class PresubmitCheckState { this.startTime, this.endTime, this.summary, + this.buildNumber, }); } @@ -38,5 +40,6 @@ extension BuildToPresubmitCheckState on bbv2.Build { startTime: startTime.toDateTime().microsecondsSinceEpoch, endTime: endTime.toDateTime().microsecondsSinceEpoch, summary: summaryMarkdown, + buildNumber: number, ); } diff --git a/app_dart/lib/src/model/firestore/presubmit_check.dart b/app_dart/lib/src/model/firestore/presubmit_check.dart index 6fc2673d1..a19d0e7ed 100644 --- a/app_dart/lib/src/model/firestore/presubmit_check.dart +++ b/app_dart/lib/src/model/firestore/presubmit_check.dart @@ -233,6 +233,14 @@ final class PresubmitCheck extends AppDocument { fields[fieldEndTime] = endTime.toValue(); } + set buildNumber(int? buildNumber) { + if (buildNumber == null) { + fields.remove(fieldBuildNumber); + } else { + fields[fieldBuildNumber] = buildNumber.toValue(); + } + } + set summary(String? summary) { if (summary == null) { fields.remove(fieldSummary); diff --git a/app_dart/lib/src/request_handlers/get_presubmit_checks.dart b/app_dart/lib/src/request_handlers/get_presubmit_checks.dart index af074107a..3f3ced517 100644 --- a/app_dart/lib/src/request_handlers/get_presubmit_checks.dart +++ b/app_dart/lib/src/request_handlers/get_presubmit_checks.dart @@ -88,6 +88,7 @@ final class GetPresubmitChecks extends ApiRequestHandler { endTime: check.endTime, status: check.status.value, summary: check.summary, + buildNumber: check.buildNumber, ), ]; diff --git a/app_dart/lib/src/service/firestore/unified_check_run.dart b/app_dart/lib/src/service/firestore/unified_check_run.dart index 6b33b9a8d..8e26dddbb 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -475,6 +475,7 @@ final class UnifiedCheckRun { presubmitGuard.failedBuilds = failed; presubmitCheck.endTime = state.endTime!; presubmitCheck.summary = state.summary; + presubmitCheck.buildNumber = state.buildNumber; } builds[state.buildName] = status; presubmitGuard.builds = builds; diff --git a/app_dart/test/model/common/presubmit_check_state_test.dart b/app_dart/test/model/common/presubmit_check_state_test.dart new file mode 100644 index 000000000..6d5a4f8ec --- /dev/null +++ b/app_dart/test/model/common/presubmit_check_state_test.dart @@ -0,0 +1,32 @@ +// Copyright 2025 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:buildbucket/buildbucket_pb.dart' as bbv2; +import 'package:cocoon_common/task_status.dart'; +import 'package:cocoon_server_test/test_logging.dart'; +import 'package:cocoon_service/src/model/common/presubmit_check_state.dart'; +import 'package:fixnum/fixnum.dart'; +import 'package:test/test.dart'; + +void main() { + useTestLoggerPerTest(); + group('PresubmitCheckState', () { + test('BuildToPresubmitCheckState extension maps build number', () { + final build = bbv2.Build( + builder: bbv2.BuilderID(builder: 'linux_test'), + status: bbv2.Status.SUCCESS, + number: 12345, + startTime: bbv2.Timestamp(seconds: Int64(1000)), + endTime: bbv2.Timestamp(seconds: Int64(2000)), + summaryMarkdown: 'Summary', + tags: [bbv2.StringPair(key: 'github_checkrun', value: '123')], + ); + + final state = build.toPresubmitCheckState(); + expect(state.buildName, 'linux_test'); + expect(state.status, TaskStatus.succeeded); + expect(state.buildNumber, 12345); + }); + }); +} diff --git a/app_dart/test/model/firestore/presubmit_check_test.dart b/app_dart/test/model/firestore/presubmit_check_test.dart index db89b824a..b2649a4fb 100644 --- a/app_dart/test/model/firestore/presubmit_check_test.dart +++ b/app_dart/test/model/firestore/presubmit_check_test.dart @@ -169,5 +169,19 @@ void main() { expect(check.status, TaskStatus.succeeded); }); + + test('buildNumber setter updates fields', () { + final check = PresubmitCheck.init( + buildName: 'linux', + checkRunId: 123, + creationTime: 1000, + ); + + check.buildNumber = 789; + expect(check.buildNumber, 789); + + check.buildNumber = null; + expect(check.buildNumber, isNull); + }); }); } diff --git a/app_dart/test/request_handlers/get_presubmit_checks_test.dart b/app_dart/test/request_handlers/get_presubmit_checks_test.dart index fd6cc77f9..df53c4037 100644 --- a/app_dart/test/request_handlers/get_presubmit_checks_test.dart +++ b/app_dart/test/request_handlers/get_presubmit_checks_test.dart @@ -85,6 +85,7 @@ void main() { startTime: 110, endTime: 120, summary: 'all good', + buildNumber: 456, ); await firestoreService.writeViaTransaction( documentsToWrites([check], exists: false), @@ -101,6 +102,7 @@ void main() { expect(checks[0].attemptNumber, 1); expect(checks[0].buildName, 'linux'); expect(checks[0].status, 'Succeeded'); + expect(checks[0].buildNumber, 456); }); test('returns multiple checks in descending order', () async { diff --git a/app_dart/test/service/firestore/unified_check_run_test.dart b/app_dart/test/service/firestore/unified_check_run_test.dart index 7db68690d..e6a7f0cf1 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -172,6 +172,7 @@ void main() { attemptNumber: 1, startTime: 2000, endTime: 3000, + buildNumber: 456, ); final result = await UnifiedCheckRun.markConclusion( @@ -194,6 +195,7 @@ void main() { ); expect(checkDoc.status, TaskStatus.succeeded); expect(checkDoc.endTime, 3000); + expect(checkDoc.buildNumber, 456); }); test( diff --git a/conductor/archive/add_build_number_20260212/index.md b/conductor/archive/add_build_number_20260212/index.md new file mode 100644 index 000000000..1b8314d4f --- /dev/null +++ b/conductor/archive/add_build_number_20260212/index.md @@ -0,0 +1,5 @@ +# Track add_build_number_20260212 Context + +- [Specification](./spec.md) +- [Implementation Plan](./plan.md) +- [Metadata](./metadata.json) diff --git a/conductor/archive/add_build_number_20260212/metadata.json b/conductor/archive/add_build_number_20260212/metadata.json new file mode 100644 index 000000000..08ff09314 --- /dev/null +++ b/conductor/archive/add_build_number_20260212/metadata.json @@ -0,0 +1,8 @@ +{ + "track_id": "add_build_number_20260212", + "type": "feature", + "status": "new", + "created_at": "2026-02-12T12:00:00Z", + "updated_at": "2026-02-12T12:00:00Z", + "description": "Add optional buildNumber to presubmitCheck and fill it on processing checkrun completed" +} diff --git a/conductor/archive/add_build_number_20260212/plan.md b/conductor/archive/add_build_number_20260212/plan.md new file mode 100644 index 000000000..b376f3088 --- /dev/null +++ b/conductor/archive/add_build_number_20260212/plan.md @@ -0,0 +1,33 @@ +# Implementation Plan: Add `buildNumber` to `PresubmitCheck` + +This plan outlines the steps to add an optional `buildNumber` field across the Cocoon backend models and ensure it is populated during the check run completion process. + +## Phase 1: Model Updates [checkpoint: 539f12a] + +This phase focuses on updating the internal and external data models to support the new `buildNumber` field. + +- [x] Task: Update `PresubmitCheckState` Model + - [x] Add `int? buildNumber` to `PresubmitCheckState` class in `app_dart/lib/src/model/common/presubmit_check_state.dart`. + - [x] Update `BuildToPresubmitCheckState` extension to map `number` from `bbv2.Build` to `buildNumber`. + - [x] Update existing tests to reflect constructor changes. +- [x] Task: Update `PresubmitCheck` Firestore Model + - [x] Add `fieldBuildNumber` constant and `buildNumber` getter/setter to `PresubmitCheck` in `app_dart/lib/src/model/firestore/presubmit_check.dart`. + - [x] Update `fromDocument`, `init`, and `toJson` (or equivalent) to handle the new field. + - [x] Add unit tests for serialization/deserialization of `buildNumber`. +- [x] Task: Update `PresubmitCheckResponse` RPC Model + - [x] Add `int? buildNumber` to `PresubmitCheckResponse` in `packages/cocoon_common/lib/src/rpc_model/presubmit_check_response.dart`. + - [x] Run `dart run build_runner build --delete-conflicting-outputs` in `packages/cocoon_common/` to regenerate JSON serialization code. + - [x] Add unit tests for the RPC model. +- [x] Task: Conductor - User Manual Verification 'Model Updates' (Protocol in workflow.md) + +## Phase 2: Backend Logic and API Updates [checkpoint: ead405d] + +This phase integrates the new field into the core logic and ensures it is returned by the API. + +- [x] Task: Update `UnifiedCheckRun.markConclusion` Logic + - [x] Update `markConclusion` in `app_dart/lib/src/service/firestore/unified_check_run.dart` to assign `state.buildNumber` to `presubmitCheck.buildNumber`. + - [x] Add/update tests in `app_dart/test/service/firestore/unified_check_run_test.dart` to verify the build number is correctly saved to Firestore. +- [x] Task: Update `GetPresubmitChecks` API Handler + - [x] Update `GetPresubmitChecks` in `app_dart/lib/src/request_handlers/get_presubmit_checks.dart` to map `PresubmitCheck.buildNumber` to `PresubmitCheckResponse.buildNumber`. + - [x] Update handler tests to verify the API response contains the build number. +- [x] Task: Conductor - User Manual Verification 'Backend Logic and API Updates' (Protocol in workflow.md) diff --git a/conductor/archive/add_build_number_20260212/spec.md b/conductor/archive/add_build_number_20260212/spec.md new file mode 100644 index 000000000..e6f483eec --- /dev/null +++ b/conductor/archive/add_build_number_20260212/spec.md @@ -0,0 +1,34 @@ +# Specification: Add `buildNumber` to `PresubmitCheck` + +## Overview +Currently, the `PresubmitCheck` model in Cocoon's Firestore and the corresponding RPC response (`PresubmitCheckResponse`) do not include the canonical LUCI build number. This makes it difficult for backend processes to uniquely identify and link to specific build attempts. This feature adds an optional `buildNumber` field to these models, as well as the intermediate `PresubmitCheckState`, ensuring it is populated when a GitHub `check_run` completion event is processed. + +## Goals +- **Improve Traceability:** Provide a direct reference to the LUCI build number in the backend data. +- **Enhanced Context:** Ensure the backend records contain the canonical build number for better auditing and debugging. +- **Future Support:** Lay the groundwork for re-running specific builds using their build number and for future UI integrations. + +## Functional Requirements +1. **Intermediate State Update:** Add a `buildNumber` field (integer, optional) to `PresubmitCheckState` in `app_dart/lib/src/model/common/presubmit_check_state.dart`. +2. **Firestore Model Update:** Add a `buildNumber` field (integer, optional) to the `PresubmitCheck` document in Firestore. +3. **RPC Model Update:** Add a `buildNumber` field (integer, optional) to the `PresubmitCheckResponse` RPC model. +4. **Data Population:** Update `UnifiedCheckRun.markConclusion` (or the relevant handler for `check_run` completion) to: + - Fetch the canonical build number from the LUCI BuildBucket API if it's not already available in the incoming `check_run` state. + - Store this `buildNumber` in the `PresubmitCheck` document. +5. **Handling Missing Data:** Older check runs or runs where the build number cannot be retrieved should have the `buildNumber` field set to `null` or omitted. + +## Non-Functional Requirements +- **Performance:** Fetching the build number from BuildBucket should not significantly delay the processing of `check_run` events. +- **Reliability:** If fetching the build number fails, the system should still record the completion status without the build number rather than failing the entire transaction. + +## Acceptance Criteria +- [ ] `PresubmitCheckState` includes a `buildNumber` field. +- [ ] `PresubmitCheck` documents in Firestore can store a `buildNumber`. +- [ ] `PresubmitCheckResponse` JSON includes a `buildNumber` field when available. +- [ ] When a `check_run` completes, the `buildNumber` is correctly retrieved and stored. +- [ ] Existing check runs without a build number continue to work (field is null). + +## Out of Scope +- **Dashboard Integration:** Updating the dashboard UI to display or use the build number is out of scope for this track. +- **Backfilling:** Backfilling build numbers for historical check runs. +- **Rerun Logic:** Implementing the "re-run by build number" functionality. diff --git a/packages/cocoon_common/lib/src/rpc_model/presubmit_check_response.dart b/packages/cocoon_common/lib/src/rpc_model/presubmit_check_response.dart index 4338d0728..4d6b9b444 100644 --- a/packages/cocoon_common/lib/src/rpc_model/presubmit_check_response.dart +++ b/packages/cocoon_common/lib/src/rpc_model/presubmit_check_response.dart @@ -10,7 +10,11 @@ import 'base.dart'; part 'presubmit_check_response.g.dart'; /// RPC model for a presubmit check. -@JsonSerializable(checked: true, fieldRename: FieldRename.snake) +@JsonSerializable( + checked: true, + fieldRename: FieldRename.snake, + includeIfNull: false, +) @immutable final class PresubmitCheckResponse extends Model { /// Creates a [PresubmitCheckResponse] with the given properties. @@ -22,6 +26,7 @@ final class PresubmitCheckResponse extends Model { this.endTime, required this.status, this.summary, + this.buildNumber, }); /// Creates a [PresubmitCheckResponse] from [json] representation. @@ -54,6 +59,9 @@ final class PresubmitCheckResponse extends Model { /// A brief summary of the check result or link to logs. final String? summary; + /// The LUCI build number. + final int? buildNumber; + @override Map toJson() => _$PresubmitCheckResponseToJson(this); } diff --git a/packages/cocoon_common/lib/src/rpc_model/presubmit_check_response.g.dart b/packages/cocoon_common/lib/src/rpc_model/presubmit_check_response.g.dart index b2f78a435..e4cbf60c6 100644 --- a/packages/cocoon_common/lib/src/rpc_model/presubmit_check_response.g.dart +++ b/packages/cocoon_common/lib/src/rpc_model/presubmit_check_response.g.dart @@ -23,6 +23,7 @@ PresubmitCheckResponse _$PresubmitCheckResponseFromJson( endTime: $checkedConvert('end_time', (v) => (v as num?)?.toInt()), status: $checkedConvert('status', (v) => v as String), summary: $checkedConvert('summary', (v) => v as String?), + buildNumber: $checkedConvert('build_number', (v) => (v as num?)?.toInt()), ); return val; }, @@ -32,6 +33,7 @@ PresubmitCheckResponse _$PresubmitCheckResponseFromJson( 'creationTime': 'creation_time', 'startTime': 'start_time', 'endTime': 'end_time', + 'buildNumber': 'build_number', }, ); @@ -41,8 +43,9 @@ Map _$PresubmitCheckResponseToJson( 'attempt_number': instance.attemptNumber, 'build_name': instance.buildName, 'creation_time': instance.creationTime, - 'start_time': instance.startTime, - 'end_time': instance.endTime, + 'start_time': ?instance.startTime, + 'end_time': ?instance.endTime, 'status': instance.status, - 'summary': instance.summary, + 'summary': ?instance.summary, + 'build_number': ?instance.buildNumber, }; diff --git a/packages/cocoon_common/test/presubmit_check_response_test.dart b/packages/cocoon_common/test/presubmit_check_response_test.dart new file mode 100644 index 000000000..d77f62bf8 --- /dev/null +++ b/packages/cocoon_common/test/presubmit_check_response_test.dart @@ -0,0 +1,41 @@ +// Copyright 2025 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_common/rpc_model.dart'; +import 'package:test/test.dart'; + +void main() { + group('PresubmitCheckResponse', () { + test('toJson and fromJson handles buildNumber', () { + final json = { + 'attempt_number': 1, + 'build_name': 'linux', + 'creation_time': 1000, + 'status': 'succeeded', + 'build_number': 456, + }; + + final response = PresubmitCheckResponse.fromJson(json); + expect(response.buildNumber, 456); + + final backToJson = response.toJson(); + expect(backToJson['build_number'], 456); + }); + + test('toJson and fromJson handles null buildNumber', () { + final json = { + 'attempt_number': 1, + 'build_name': 'linux', + 'creation_time': 1000, + 'status': 'succeeded', + }; + + final response = PresubmitCheckResponse.fromJson(json); + expect(response.buildNumber, isNull); + + final backToJson = response.toJson(); + expect(backToJson.containsKey('build_number'), isFalse); + }); + }); +}