Refactor writing of package config in tests#163734
Merged
sigurdm merged 13 commits intoflutter:masterfrom Feb 28, 2025
Merged
Conversation
github-actions
bot
added
tool
sigurdm
force-pushed
the
package_conf_tests
branch
from
8683870 to
bd75100
Compare
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
sigurdm
force-pushed
the
package_conf_tests
branch
from
bd75100 to
e6f7027
Compare
matanlurey
approved these changes
Feb 24, 2025
Contributor
matanlurey
left a comment
matanlurey
left a comment
There was a problem hiding this comment.
LGTM, Approval.
A few minor suggestions that are not blocking.
| /// [otherLibs] maps other package names to their `rootUri` relative to `directory`. | ||
| /// | ||
| /// Returns the `File` Object representing the package config. | ||
| File writePackageConfigFile({ |
Contributor
There was a problem hiding this comment.
This is more of a clarification question than a block:
Why do this instead of just Iterable<Package> packages (from package:package_config)?
Is there something that is gained other than a bit of code tersity?
Contributor
Author
There was a problem hiding this comment.
I did not really consider using a list of Package but looking at the constructor, it requires repeating the root uri (the location of the package config)
I think that makes it kinda unergonomic.
Also the Package.packageUri getter is always absolute, meaning no easy way to test relative paths.
I think I'll keep this for now, though I can see how a list of packages could make sense
| /// | ||
| /// Returns the `File` Object representing the package config. | ||
| File writePackageConfigFile({ | ||
| Directory? directory, |
Contributor
There was a problem hiding this comment.
I would prefer to make this explicit (required non-null) instead of falling back to globals.fs.currentDirectory; the tests that want this behavior can write it themselves IMO.
| /// Returns the `File` Object representing the package config. | ||
| File writePackageConfigFile({ | ||
| Directory? directory, | ||
| String mainLibName = 'my_app', |
Contributor
There was a problem hiding this comment.
Ditto, I would make this required instead of assuming a default.
github-actions
bot
added
a: tests
Contributor
Author
|
@jmagman sorry, I somehow made a bad merge that added you as a reviewer.. |
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 28, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 28, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 28, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 28, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 28, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 1, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 1, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 2, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 2, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 2, 2025
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Mar 2, 2025
Roll Flutter from 2e570ca to 842db35 (59 revisions) flutter/flutter@2e570ca...842db35 2025-03-02 [email protected] Roll Skia from ad64415050aa to 101eee8fce59 (1 revision) (flutter/flutter#164449) 2025-03-02 [email protected] Roll Fuchsia Linux SDK from ln3joxJfRN2XGhvCv... to AO1KirSDI7-MVYNPN... (flutter/flutter#164440) 2025-03-02 [email protected] android: Clean up gen_snapshot artifact build (flutter/flutter#164418) 2025-03-02 [email protected] Start using `bin/cache/engine.{stamp|realm}` instead of `bin/internal/engine.{realm|version}`. (flutter/flutter#164352) 2025-03-01 [email protected] Add macos/android_debug_unopt to local_engine.json (flutter/flutter#164410) 2025-03-01 [email protected] Delete unused build archive targets (flutter/flutter#164414) 2025-03-01 [email protected] Roll Fuchsia Linux SDK from QMun2itYrV_zUYrvW... to ln3joxJfRN2XGhvCv... (flutter/flutter#164423) 2025-03-01 [email protected] Roll Skia from ac14158663ea to ad64415050aa (1 revision) (flutter/flutter#164413) 2025-03-01 [email protected] Update linux_host_engine.json ci/host_release description (flutter/flutter#164402) 2025-03-01 [email protected] In update_engine_version_test.dart, do not populate the test environment with the host platform environment (flutter/flutter#164395) 2025-03-01 [email protected] Roll-forward #164317: Use `bin/cache/engine.stamp` (flutter/flutter#164401) 2025-02-28 [email protected] Make pressing and moving on CupertinoButton closer to native behavior. (flutter/flutter#161731) 2025-02-28 [email protected] Roll Skia from 4005ba3ca7b6 to ac14158663ea (7 revisions) (flutter/flutter#164404) 2025-02-28 [email protected] [macOS] Prepare FlutterKeyboardManager for multi-view (flutter/flutter#163962) 2025-02-28 [email protected] Add PlatformDispatcher.engineId (flutter/flutter#163476) 2025-02-28 [email protected] Move `integration_test.FlutterDeviceScreenshotTest` to the framework slow shard (flutter/flutter#164398) 2025-02-28 [email protected] Fix: Update DelegatedTransition animation parameter correctly (flutter/flutter#163853) 2025-02-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Write an identical value to `bin/cache/engine.stamp` to prepare for migration (#164317)" (flutter/flutter#164396) 2025-02-28 [email protected] Add action for configuring default action of EditableText.onTapUpOutside (flutter/flutter#162575) 2025-02-28 [email protected] Align jvmTarget usages across codebase, while editing build.gradle files align them with android version documentation (flutter/flutter#164200) 2025-02-28 [email protected] Roll Packages from 01d3d5c to 70b41e1 (5 revisions) (flutter/flutter#164380) 2025-02-28 [email protected] [tool] Allow using archiveName in android bundle build (flutter/flutter#162390) 2025-02-28 [email protected] Fix incorrectly checking for invalid environment variables in the tool (flutter/flutter#164101) 2025-02-28 [email protected] Support forward and back buttons (flutter/flutter#164356) 2025-02-28 [email protected] Fix window creation callback for multi-window (flutter/flutter#164353) 2025-02-28 [email protected] Fix flutter doctor usage of eglinfo in failure cases. (flutter/flutter#164334) 2025-02-28 [email protected] Refactor writing of package config in tests (flutter/flutter#163734) 2025-02-28 [email protected] Fixed the issue that Slider's secondaryTrackValue is not updated. (flutter/flutter#163996) 2025-02-28 [email protected] Roll pub packages (flutter/flutter#164357) 2025-02-28 [email protected] Remove `Mac mac_unopt` presubmit retry count (flutter/flutter#164350) 2025-02-28 [email protected] Roll Fuchsia Linux SDK from 1elkOxihZuTEiTXzY... to QMun2itYrV_zUYrvW... (flutter/flutter#164351) 2025-02-28 [email protected] Drag handles only need to be tested on mobile platforms. (flutter/flutter#163723) 2025-02-28 [email protected] [Android] Use java for looking up Android API level. (flutter/flutter#163558) 2025-02-28 [email protected] Write an identical value to `bin/cache/engine.stamp` to prepare for migration (flutter/flutter#164317) 2025-02-27 [email protected] Wires up expanded state in web engine (flutter/flutter#164048) 2025-02-27 [email protected] Remove Cheserton's File (flutter/flutter#164340) 2025-02-27 [email protected] Replace update semantics handler with signal. (flutter/flutter#163583) 2025-02-27 [email protected] Split up the conical gradient fragment shader (flutter/flutter#164058) 2025-02-27 [email protected] Roll Skia from c16b145749d4 to 4005ba3ca7b6 (3 revisions) (flutter/flutter#164339) 2025-02-27 [email protected] Delete and update stale documentation regarding engine/engine hash. (flutter/flutter#164324) 2025-02-27 [email protected] Document how `engine.version` (is/will be) computed (flutter/flutter#164335) 2025-02-27 [email protected] Update conductor to write engine.version file (flutter/flutter#163350) 2025-02-27 [email protected] remove last usages of min/compile/target SdkVersion, align sourceCompatibility across repo and update android version documentation (flutter/flutter#164198) 2025-02-27 [email protected] Update links to the `flutter/engine` repository for the monorepo. (flutter/flutter#164328) 2025-02-27 [email protected] Add empty `io.flutter.app.FlutterApplication` to give deprecation notice, and un-break projects that have not migrated (flutter/flutter#164233) 2025-02-27 [email protected] Revert dart sdks that were causing dartaotruntime issues in g3 (flutter/flutter#164307) ...
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 20, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 20, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seems like each test had its own way of doing this.
Extracted from #160443