Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

liyuqian
Copy link
Contributor

Also enable shell_benchmarks in postsubmit release benchmarks.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

This won't actually terminate the process in the wild when the embedder manages to pass a non-whitelisted flag to the shell.

@liyuqian
Copy link
Contributor Author

@chinmaygarde : the new code still fails. I wonder if ASSERT_DEATH works in release mode?

@liyuqian
Copy link
Contributor Author

@chinmaygarde , I've found the issue: there's also a whitelist unit test. Although the failure is about the blacklisted flags, it now comes from the whitelist unit test instead of the blacklist unit test. Fixing whitelist unit test seems to have fixed the issue.

RunEngineBenchmarks(build_dir, engine_filter)

if 'engine' in types or 'font-subset' in types:
if ('engine' in types or 'font-subset' in types) and args.variant != 'host_release':
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dnfield : the font-subset doesn't seem to be available in release mode. I also wonder if it's available in the profile mode. Is it debug mode only?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think only the debug mode asset is built - it should be possible to build the release mode, but not important to test it in release mode.

@liyuqian liyuqian force-pushed the release_test branch 2 times, most recently from d874224 to 6377cb0 Compare March 31, 2020 22:51
liyuqian added 7 commits April 1, 2020 09:51
Also enable shell_benchmarks in postsubmit release benchmarks.
That's the intended behavior according to Ben.
Chinmay said terminate on release is the intended behavior.
@liyuqian liyuqian added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 1, 2020
@fluttergithubbot
Copy link
Contributor

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

  • This pull request has changes requested by @chinmaygarde. Please resolve those before re-applying the label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 1, 2020
@liyuqian liyuqian added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 1, 2020
@fluttergithubbot fluttergithubbot merged commit a82343b into flutter:master Apr 2, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 2, 2020
@liyuqian liyuqian deleted the release_test branch April 10, 2020 00:50
goderbauer pushed a commit to goderbauer/engine that referenced this pull request Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants