Skip to content

improve web benchmark error reporting#51490

Merged
yjbanov merged 1 commit intoflutter:masterfrom
yjbanov:web-bench-errors
Feb 28, 2020
Merged

improve web benchmark error reporting#51490
yjbanov merged 1 commit intoflutter:masterfrom
yjbanov:web-bench-errors

Conversation

@yjbanov
Copy link
Copy Markdown
Contributor

@yjbanov yjbanov commented Feb 26, 2020

Description

  • Aggressively stop benchmarks in the presence of errors
  • Forward errors to localhost for forwarding to test results
  • Wrap the checkbox in Material and Directionality. Otherwise it fails assertions in debug mode (probably for good reason). This was not noticed in benchmarks because we don't run them in debug mode.

@yjbanov yjbanov requested a review from mdebbar February 26, 2020 18:52
@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Feb 26, 2020
@fluttergithubbot
Copy link
Copy Markdown
Contributor

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Comment on lines +590 to +593
// Don't keep on truckin' if there's an error.
if (_hasErrored) {
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of checking for _hasErrored in multiple places, what if we check for it in _shouldContinue() and rely on existing logic to stop pumping frames?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can't think of a clean way to use it. Currently _shouldContinue depends on everything being healthy. For example, it extracts the profile from rendered frames. It only communicates two signals: "continue running" and "benchmark finished successfully". We'd have to add a third value communicating "benchmark errored and must halt". When an error happens the system could be in a corrupted state, in which case our job is to halt immediately and report the error before it's obscured by something else. IOW I think they have sufficiently different roles to warrant separate codepaths and separate signals.

@fluttergithubbot
Copy link
Copy Markdown
Contributor

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@yjbanov yjbanov merged commit cd0fbd3 into flutter:master Feb 28, 2020
yjbanov added a commit to yjbanov/flutter that referenced this pull request Feb 28, 2020
@mdebbar
Copy link
Copy Markdown
Contributor

mdebbar commented Feb 28, 2020

LGTM

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. f: material design flutter/packages/flutter/material repository.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants