Skip to content
This repository was archived by the owner on Jun 13, 2024. It is now read-only.

Use writeTimelineToFile #470

Merged
merged 3 commits into from
Mar 31, 2021
Merged

Use writeTimelineToFile #470

merged 3 commits into from
Mar 31, 2021

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Mar 31, 2021 •
edited
Loading

writeSummaryToFile is being deprecated in flutter/flutter#79310

@dnfield dnfield requested a review from guidezpl March 31, 2021 17:40
@jmagman jmagman mentioned this pull request Mar 31, 2021
Closed
guidezpl
guidezpl approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member

@guidezpl guidezpl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  _    ___ _____ __  __ 
 | |  / __|_   _|  \/  |
 | |_| (_ | | | | |\/| |
 |____\___| |_| |_|  |_|

Regarding the tests, the updated goldens LGTM. You may want to temporarily disable the integration test for web due to https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/commands/test.dart#L411

@dnfield
Copy link
Contributor Author

dnfield commented Mar 31, 2021

@guidezpl - do you know what's going on with the CI failures?

@guidezpl
Copy link
Member

Btw the goldens can be downloaded from https://github.com/flutter/gallery/suites/2391882323/artifacts/50842950 and replace the existing ones.

@dnfield
Copy link
Contributor Author

dnfield commented Mar 31, 2021

Added the goldens.

@dnfield
Copy link
Contributor Author

dnfield commented Mar 31, 2021

The integration test looks like it was broken by flutter/flutter#74236 - @jiahaog

@dnfield
Copy link
Contributor Author

dnfield commented Mar 31, 2021

I'm going to land this since it's unrelated and it will help get a different test re-enabled.

@dnfield
Copy link
Contributor Author

dnfield commented Mar 31, 2021

Oh. I don't have permission to land it while the status is broken haha.

@dnfield
Copy link
Contributor Author

dnfield commented Mar 31, 2021

Ok - the test in question isn't actually depending on dart:ui or package:integration_test, so I'm switching it to just use dart test, which works. Flutter is special casing testsin the integeration_test directory now.

@dnfield dnfield merged commit 2bb63d1 into master Mar 31, 2021
@dnfield dnfield deleted the dnfield-patch-1 branch March 31, 2021 18:43
@jiahaog
Copy link
Member

jiahaog commented Mar 31, 2021

The integration test looks like it was broken by flutter/flutter#74236 - @jiahaog

@dnfield correct me if I'm wrong, you mean flutter/flutter#76200 for this failure right

@dnfield
Copy link
Contributor Author

dnfield commented Mar 31, 2021

Ahh yes, I think I pasted or blamed the wrong PR.

@jiahaog
Copy link
Member

jiahaog commented Apr 1, 2021

To avoid future confusion here, I suggest moving the test out of the integration_test/ directory, since it looks like the test is meant to be executed on the host. We could do either of the following:

A. Move$repo_root/integration_test/gallery_compile_test.dart to something like test/gallery_integration_test.dart. Drawback: flutter test will include this file as well, which may slow things down
B. A, and we tag the test appropriately with something like "slow". In that way, we can pass -x to flutter test to exclude it if desired - https://pub.dev/packages/test#tagging-tests

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@guidezpl guidezpl guidezpl approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dnfield @guidezpl @jiahaog