Skip to content
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

flutter tool: log dart2wasm vs dart2js as appropriate during build #124165

Merged
merged 7 commits into from
Apr 6, 2023
Merged

flutter tool: log dart2wasm vs dart2js as appropriate during build #124165

merged 7 commits into from
Apr 6, 2023

Conversation

kevmoo
Copy link
Contributor

@kevmoo kevmoo commented Apr 4, 2023

Fixes #124164

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 4, 2023
@flutter-dashboard
Copy link

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 (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

christopherfujino
christopherfujino approved these changes Apr 4, 2023
View reviewed changes
Copy link
Member

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 4, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 5, 2023

auto label is removed for flutter/flutter, pr: 124165, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@jmagman jmagman mentioned this pull request Apr 5, 2023
Merged
8 tasks
@jmagman
Copy link
Member

jmagman commented Apr 5, 2023 •
edited
Loading

It looks like this pull request may not have tests.

PR at #124179 shows how something like this could be tested with mocks.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 5, 2023

auto label is removed for flutter/flutter, pr: 124165, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 5, 2023

auto label is removed for flutter/flutter, pr: 124165, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 5, 2023

auto label is removed for flutter/flutter, pr: 124165, due to - The status or check suite Mac build_ios_framework_module_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

@kevmoo
Copy link
Contributor Author

kevmoo commented Apr 5, 2023

Now with tests! Thanks @jmagman !

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@jmagman
Copy link
Member

jmagman commented Apr 6, 2023

Now with tests! Thanks @jmagman !

This could use a test where isWasm: false to cover the other branch 🙂

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 6, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 6, 2023

auto label is removed for flutter/flutter, pr: 124165, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 6, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 6, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 6, 2023

auto label is removed for flutter/flutter, pr: 124165, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 6, 2023
@auto-submit auto-submit bot merged commit 929a5bd into master Apr 6, 2023
@auto-submit auto-submit bot deleted the dart2wasm_logging branch April 6, 2023 05:34
@zanderso zanderso mentioned this pull request Apr 6, 2023
Merged
zanderso added a commit that referenced this pull request Apr 6, 2023
@zanderso
Revert "flutter tool: log dart2wasm vs dart2js as appropriate during …
8f1308d 
…build (#124165)"

This reverts commit 929a5bd.
@zanderso zanderso mentioned this pull request Apr 6, 2023
Merged
@auto-submit auto-submit bot mentioned this pull request Apr 6, 2023
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 6, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 6, 2023
@engine-flutter-autoroll
929a5bd flutter tool: log dart2wasm vs dart2js as appropriate during …
85ed270 
…build (flutter/flutter#124165)
@kevmoo kevmoo mentioned this pull request Apr 6, 2023
Merged
kevmoo added a commit that referenced this pull request Apr 6, 2023
@kevmoo
flutter tool: log dart2wasm vs dart2js as appropriate during build
2d4c682 
Relands   #124165 - 929a5bd
Unreverts #124305 - 822632b
exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023
@kevmoo @exaby73
flutter tool: log dart2wasm vs dart2js as appropriate during build (f…
75026e1 
…lutter#124165)

flutter tool: log dart2wasm vs dart2js as appropriate during build
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
929a5bd flutter tool: log dart2wasm vs dart2js as appropriate during …
f2d75fd 
…build (flutter/flutter#124165)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
929a5bd flutter tool: log dart2wasm vs dart2js as appropriate during …
5c66f47 
…build (flutter/flutter#124165)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christopherfujino christopherfujino christopherfujino approved these changes

@eyebrowsoffire eyebrowsoffire Awaiting requested review from eyebrowsoffire

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

flutter tool, web: should we report dart2wasm time vs dart2js time...when doing wasm things?
3 participants
@kevmoo @jmagman @christopherfujino