Skip to content

print intermediate and raw A/B results when not silent #54676

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Apr 16, 2020
Merged

print intermediate and raw A/B results when not silent #54676

merged 5 commits into from
Apr 16, 2020

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Apr 13, 2020

Description

Some tests can take a while to run. If --silent is not specified, print A/B results for the benchmark iterations that ran so far. It can also be used to set --ab to something very high, like 100, and then stop the benchmark manually.

@fluttergithubbot fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Apr 13, 2020
@fluttergithubbot
Copy link
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 on the #hackers channel in Chat.

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

@yjbanov yjbanov requested a review from liyuqian April 13, 2020 18:29
liyuqian
liyuqian approved these changes Apr 13, 2020
View reviewed changes
Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

LGTM. Yes, the test usually takes a very long time for a single run (e.g., 30 seconds) and A/B test is printing very few things regarding that time.

@liyuqian
Copy link
Contributor

BTW, I just realized that the original PR #54494 doesn't have a unit test. Should this neat A/B test have some unit tests just to make sure that it won't be accidentally broken in the future?

@yjbanov yjbanov force-pushed the intermediate-ab-results branch from fae3cf2 to 90e7064 Compare April 16, 2020 00:02
@yjbanov
Copy link
Contributor Author

yjbanov commented Apr 16, 2020

BTW, I just realized that the original PR #54494 doesn't have a unit test. Should this neat A/B test have some unit tests just to make sure that it won't be accidentally broken in the future?

Done.

@yjbanov
Copy link
Contributor Author

yjbanov commented Apr 16, 2020

Oops! Hold on! I rebased on top of a wrong branch 🤦

@yjbanov yjbanov force-pushed the intermediate-ab-results branch from 90e7064 to 9f897ec Compare April 16, 2020 00:10
@yjbanov yjbanov mentioned this pull request Apr 16, 2020
Closed
@yjbanov yjbanov changed the title print intermediate A/B results when not silent print intermediate and raw A/B results when not silent Apr 16, 2020
@yjbanov yjbanov mentioned this pull request Apr 16, 2020
Closed
@yjbanov
Copy link
Contributor Author

yjbanov commented Apr 16, 2020

This is ready for re-review. PTAL.

@yjbanov yjbanov force-pushed the intermediate-ab-results branch from e9851a3 to 7e3b116 Compare April 16, 2020 19:37
@liyuqian
Copy link
Contributor

Still LGTM!

@yjbanov yjbanov merged commit 2080350 into flutter:master Apr 16, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@liyuqian liyuqian liyuqian approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
c: contributor-productivity Team-specific productivity, code health, technical debt.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yjbanov @fluttergithubbot @liyuqian @googlebot