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.

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

Add integration test for asset transformation feature #145715

Merged

Conversation

andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Mar 25, 2024

In service of #143348

This adds a simple integration test for the new asset transformation feature.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@andrewkolos andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 25, 2024
@github-actions github-actions bot added a: desktop Running on desktop and removed tool Affects the "flutter" command-line tool. See also t: labels. labels Mar 25, 2024
@andrewkolos andrewkolos force-pushed the asset-transformers-integration-testing branch from 319e29c to dd5271f Compare March 25, 2024 18:30
@andrewkolos
Copy link
Contributor Author

Looks like using vector_graphics_compiler (as a transformer) and vector_graphics for seeing if the asset was transformed correctly by trying to render it won't work.

vector_graphics has a dependency on package http ^1.0.0, but googleapis_auth depends on ^0.13.0

While it would have been nice to use a more real-world-style example in the integration test, adding a bunch of transitive dependencies probably wasn't the right move here.

I'm considering instead using a contrived local path dependency as a transformer for this test.

@andrewkolos andrewkolos force-pushed the asset-transformers-integration-testing branch from 679f61a to e066881 Compare April 4, 2024 03:10
@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 4, 2024
@andrewkolos
Copy link
Contributor Author

I ran this in the try pool: https://chromium-swarm.appspot.com/task?id=68c50593530ecb10

@andrewkolos andrewkolos marked this pull request as ready for review April 5, 2024 03:13
@christopherfujino
Copy link
Member

christopherfujino commented Apr 5, 2024

I ran this in the try pool: https://chromium-swarm.appspot.com/task?id=68c50593530ecb10

This looks like Linux analyze?

.ci.yaml Outdated Show resolved Hide resolved
TESTOWNERS Outdated Show resolved Hide resolved
.ci.yaml Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the a: desktop Running on desktop label Apr 5, 2024
@@ -0,0 +1,43 @@
# Miscellaneous
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, this seems like a decent idea, but perhaps there is value is keeping the project on repo—we could use it as part of the documentation for the feature on flutter.dev. For example we do this for flavors:

For examples of build flavors for iOS, macOS, and Android, check out the integration test samples in the Flutter repo.

I imagine myself writing a "how to write your own asset-transforming package" section the flutter.dev documentation for the feature, and users might appreciate being able to see a complete example (and not just a snippet). Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add this to https://github.com/flutter/samples.

There is a baseline cost to every flutter project that we keep in the flutter/flutter repo (both pubspec deps and also maintaining the platform-specific directories.

Copy link
Member

Choose a reason for hiding this comment

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

If you go with generating this on the fly, I would recommend not using dart create to create the dart transformer project, but instead just programmatically creating the files you need, so that upstream changes to the dart create ... output won't break the flutter/flutter tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL of the existence of flutter/samples.

I'll 1) proceed with Jenn's suggestion and 2) consider adding a sample to flutter/samples when writing the flutter.dev documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this seems like a decent idea, but perhaps there is value is keeping the project on repo—we could use it as part of the documentation for the feature on flutter.dev. For example we do this for flavors:

For examples of build flavors for iOS, macOS, and Android, check out the integration test samples in the Flutter repo.

I imagine myself writing a "how to write your own asset-transforming package" section the flutter.dev documentation for the feature, and users might appreciate being able to see a complete example (and not just a snippet). Let me know what you think.

Have you reached out to DevRel? They could help with messaging this. In any case, at the point you need a complete example for documentation, and not integration tests, I agree that samples would be a better spot for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have you reached out to DevRel? They could help with messaging this.

Good idea

@christopherfujino
Copy link
Member

@andrewkolos what's the status on this PR?

@andrewkolos andrewkolos force-pushed the asset-transformers-integration-testing branch from 01f65a1 to 9a1217c Compare April 22, 2024 15:55
@andrewkolos andrewkolos added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Apr 22, 2024
@andrewkolos
Copy link
Contributor Author

@andrewkolos what's the status on this PR?

@christopherfujino, IIRC all comments have been addressed and this is ready for review.

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

@auto-submit auto-submit bot merged commit 290dadb into flutter:master Apr 26, 2024
142 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 27, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Apr 27, 2024
flutter/flutter@2e80670...f9933b6

2024-04-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5205e3683a0a to 20fb62ba1455 (1 revision) (flutter/flutter#147449)
2024-04-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from e14649ea0c80 to 5205e3683a0a (1 revision) (flutter/flutter#147448)
2024-04-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 87f489c1bed4 to e14649ea0c80 (1 revision) (flutter/flutter#147446)
2024-04-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from cecf5aa8a778 to 87f489c1bed4 (1 revision) (flutter/flutter#147445)
2024-04-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8af10eba3ef3 to cecf5aa8a778 (1 revision) (flutter/flutter#147444)
2024-04-27 chris@bracken.jp [macOS] Eliminate flutter_gallery_macos__start_up benchmark (flutter/flutter#147442)
2024-04-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from bc055398f42a to 8af10eba3ef3 (1 revision) (flutter/flutter#147441)
2024-04-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from c410180e5bba to bc055398f42a (7 revisions) (flutter/flutter#147440)
2024-04-26 sokolovskyi.konstantin@gmail.com Add tests for character_activator.0.dart API example. (flutter/flutter#147384)
2024-04-26 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.2 to 3.25.3 (flutter/flutter#147437)
2024-04-26 andrewrkolos@gmail.com Add integration test for asset transformation feature (flutter/flutter#145715)
2024-04-26 Dhankechakishan@gmail.com Added missing tests for Table api example `table.0.dart`. (flutter/flutter#147318)
2024-04-26 andrewrkolos@gmail.com Catch any `FileSystemException` thrown when trying to read the template manifest during `flutter create` (flutter/flutter#145620)
2024-04-26 102401667+Dimilkalathiya@users.noreply.github.com fixes `CupertinoFullscreenDialogTransition` leaks (flutter/flutter#147168)
2024-04-26 leroux_bruno@yahoo.fr Fix helperMaxLines and errorMaxLines documentation (flutter/flutter#147409)
2024-04-26 gspencergoog@users.noreply.github.com Refactor route focus node creation (flutter/flutter#147390)
2024-04-26 engine-flutter-autoroll@skia.org Roll Packages from fde908d to dd01140 (5 revisions) (flutter/flutter#147420)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@andrewkolos andrewkolos deleted the asset-transformers-integration-testing branch April 29, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants