Repository navigation
[tool] Deduplicate fixture builders in build_swift_package_test.dart - #190624
Conversation
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
There was a problem hiding this comment.
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.
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 PTAL? |
|
@vashworth fixed! PTAL! |
cbracken
left a comment
There was a problem hiding this comment.
Agreed with @vashworth this could probably go further but this addresses my comments.
Replaces 17 identical occurrences of boilerplate
BuildSwiftPackageUtilsdependency injection constructor blocks with a single file-local factory helper._createTestUtilshelper using named required parameters to match suite idioms.Pre-launch Checklist
///).