Skip to content

[tool] Deduplicate fixture builders in build_swift_package_test.dart - #190624

Merged
auto-submit[bot] merged 6 commits into
flutter:masterfrom
kevmoo:deslop-audit
Aug 17, 2026
Merged

auto-submit[bot] merged 6 commits into
flutter:masterfrom
kevmoo:deslop-audit

Conversation

@kevmoo

@kevmoo kevmoo commented Aug 5, 2026 •

Copy link
Copy Markdown
Contributor

Replaces 17 identical occurrences of boilerplate BuildSwiftPackageUtils dependency injection constructor blocks with a single file-local factory helper.

  • Adds top-level private _createTestUtils helper using named required parameters to match suite idioms.
  • Removes over 140 lines of duplicate test setup code while maintaining 100% test behavior parity.

Pre-launch Checklist

Replaces 17 identical occurrences of boilerplate `BuildSwiftPackageUtils` dependency injection constructor blocks with a single file-local factory helper.

* Adds top-level private `_createTestUtils` helper using named required parameters to match suite idioms.
* Removes over 140 lines of duplicate test setup code while maintaining 100% test behavior parity.
* Formats invocations to adhere to the 80-column line length lint and specifies explicit types for local variables.

No issue (purely internal test suite duplication cleanup; no functional or behavioral production changes).

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
- [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is [test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
@github-actions github-actions Bot added tool Affects the "flutter" command-line tool. See also t: labels. team-ios Owned by iOS platform team team-macos Owned by the macOS platform team labels Aug 5, 2026
@kevmoo
kevmoo marked this pull request as ready for review August 5, 2026 18:00
@kevmoo
kevmoo requested review from a team as code owners August 5, 2026 18:00

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the build_swift_package_test.dart file by introducing a helper method, _createTestUtils, to instantiate BuildSwiftPackageUtils, which eliminates redundant setup code across numerous test cases. Feedback recommends removing the optional engineArtifactPath parameter from this helper and referencing the existing top-level constant directly to prevent state duplication.

Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
@kevmoo kevmoo added the CICD Run CI/CD label Aug 5, 2026
@vashworth
vashworth self-requested a review August 6, 2026 21:32
Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
@cbracken

cbracken commented Aug 10, 2026 •

Copy link
Copy Markdown
Member

[X] I listed at least one issue that this PR fixes in the description above.

Is it hidden somewhere other than the commit comment? 😉 I just leave that box unchecked if I have no issue and this isn't really worth an issue, it's just cleanup.

More of an uber-pedantic nit but flutter doesn't use "conventional commit" formatting for commit comments. We don't say not to, but it's definitely not what we normally do, so it's a bit out of place; looks like the title was fixed up though . There were a few others like this last week but figured they were one-offs. Not an issue with the overall PR (which in this case matches the usual style) -- just something I noticed on the individual commits within the PR and some of the PRs last week.

@cbracken cbracken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall LGTM modulo nits.

@kevmoo

kevmoo commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@cbracken PTAL?

Comment thread packages/flutter_tools/test/commands.shard/hermetic/build_swift_package_test.dart Outdated
@kevmoo

kevmoo commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@vashworth fixed! PTAL!

@cbracken cbracken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed with @vashworth this could probably go further but this addresses my comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD team-ios Owned by iOS platform team team-macos Owned by the macOS platform team tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants