FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

188110 failure Dashboard Checks as soon as 1 test fails by ievdokdm · Pull Request #5095 · flutter/cocoon · GitHub

/ cocoon Public

188110 failure Dashboard Checks as soon as 1 test fails - #5095

Open
ievdokdm wants to merge 1 commit into
flutter:mainfrom
ievdokdm:188110-fail
ConversationCommits (1)ChecksFiles changed
Open

188110 failure Dashboard Checks as soon as 1 test fails#5095
ievdokdm wants to merge 1 commit into
flutter:mainfrom
ievdokdm:188110-fail

Conversation

ievdokdm commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

flutter-dashboard Bot added the CICD Run CI/CD label Jun 30, 2026
gemini-code-assist Bot reviewed Jun 30, 2026
View reviewed changes

gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code Review

This pull request introduces PresubmitGuardState to track the previous and current states (remaining and failed jobs) of a CI stage, updating CiStaging, UnifiedCheckRun, and Scheduler accordingly. This allows unified check runs to fail immediately on the first failed job. Additionally, GuardStatus.calculate is updated to return a failed status immediately if there are any failed builds. The review feedback points out a critical correctness issue in scheduler.dart where the merge queue guard could be prematurely unlocked on successful jobs in unified check runs because the pending check is skipped even when no failures have occurred yet. A code suggestion is provided to ensure we only skip waiting if there is an active failure.

Comment thread app_dart/lib/src/service/scheduler.dart Outdated

ievdokdm commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

gemini-code-assist Bot reviewed Jul 1, 2026
View reviewed changes

gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code Review

This pull request refactors the presubmit job tracking by replacing PresubmitCompletedJob with PresubmitJobState and introducing PresubmitGuardState to track previous and current job states. It also updates the scheduler to process job status updates through cleaner, flow-specific helper methods and modifies GuardStatus.calculate to fail immediately when any build fails. The review feedback highlights two critical issues: a potential null-pointer exception in scheduler.dart when accessing conclusion.checkRunGuard during an internal error, and an incorrect method invocation name in the generated mock file mocks.mocks.dart.

Comment thread app_dart/lib/src/service/scheduler.dart
Comment thread packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart
ievdokdm added CICD Run CI/CD and removed CICD Run CI/CD labels Jul 1, 2026
eyebrowsoffire requested changes Jul 7, 2026
View reviewed changes
Comment thread app_dart/lib/src/model/common/presubmit_job_state.dart Outdated
Comment thread app_dart/lib/src/model/common/presubmit_job_state.dart Outdated
ievdokdm added autosubmit Merge PR when tree becomes green via auto submit App. CICD Run CI/CD and removed CICD Run CI/CD labels Jul 7, 2026
auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App. label Jul 7, 2026

auto-submit Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/cocoon/5095, because This PR has not met approval requirements for merging. Changes were requested by {eyebrowsoffire}, please make the needed changes and resubmit this PR.
The PR author is a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a member of flutter-hackers before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

ievdokdm requested a review from eyebrowsoffire July 7, 2026 19:48
ievdokdm force-pushed the 188110-fail branch 2 times, most recently from 9f53437 to 4ceb509 Compare July 9, 2026 22:06
jtmcdole requested changes Jul 14, 2026
View reviewed changes

jtmcdole left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't want to look at a "+826 / -660" change without a statemachine doc. There is a lot of diff here for "fail quickly".

Comment thread app_dart/lib/src/model/common/presubmit_guard_conclusion.dart
Comment on lines +35 to +54
/// Represents the current state of jobs in a CI stage.
class PresubmitGuardState {
final int remaining;
final int failed;

const PresubmitGuardState({required this.remaining, required this.failed});

@override
bool operator ==(Object other) =>
identical(this, other) ||
(other is PresubmitGuardState &&
other.remaining == remaining &&
other.failed == failed);

@override
int get hashCode => Object.hash(remaining, failed);

@override
String toString() => 'PresubmitGuardState("$remaining", "$failed")';
}

jtmcdole Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could just be:

typedef PresubmitGuardState = ({int remaining, int failed});

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jtmcdole jtmcdole requested changes
eyebrowsoffire Awaiting requested review from eyebrowsoffire
+1 more reviewer
gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

CICD Run CI/CD

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Show some indication of failure as soon as 1 test fails for Dashboard Checks

3 participants

Back | FazBrowse Home | New Git URL