[quick_actions]refactor into smaller components and 100% unit test coverage#6212
Conversation
hellohuanlin
force-pushed
the
quick_actions_refactor_with_100_coverage
branch
2 times, most recently
from
1866b73 to
0d7592a
Compare
|
|
||
| @implementation FLTShortcutStateManager | ||
|
|
||
| - (void)setShortcutItems:(NSArray *)items { |
There was a problem hiding this comment.
these 2 functions are directly moved from FLTQuickActionsPlugin.m file.
hellohuanlin
force-pushed
the
quick_actions_refactor_with_100_coverage
branch
from
0d7592a to
d14a56b
Compare
hellohuanlin
force-pushed
the
quick_actions_refactor_with_100_coverage
branch
from
d14a56b to
79adeca
Compare
hellohuanlin
changed the title
hellohuanlin
force-pushed
the
quick_actions_refactor_with_100_coverage
branch
from
a73edd9 to
317b356
Compare
packages/quick_actions/quick_actions_ios/ios/Classes/FLTQuickActionsPlugin.m
Outdated
Show resolved
Hide resolved
jmagman
left a comment
jmagman
left a comment
There was a problem hiding this comment.
LGTM with nit about fixtures files.
@stuartmorgan do you think this is worth a version bump?
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| #import "Fixtures.h" |
There was a problem hiding this comment.
Instead of making a class for this can you just instantiate them in the tests that need them? I would rather it be repeated than to need to swap between files to see the expected and actual results. And I definitely don't care if it's repeated between test files.
There was a problem hiding this comment.
To clarify, there are 3 options here:
- put these data under a separate fixtures file (to be reused by all)
- put these data as helpers in each test file (to be reused by tests within the test file)
- put these data inside each test function (no reuse)
Are you referring to option 2 or 3?
I don't have a strong preference here (art, not science).
There was a problem hiding this comment.
^ feel free to chime in @cyanglaz and @stuartmorgan. Let's decide on a convention and use it onwards.
There was a problem hiding this comment.
I was referring to 2 but if I were writing these myself I would do 3. Art not science, as you say. I don't have a preference, I just think they should be in the same file to avoid
“OK, this is a FrobberTest, where’s that defined … oh, this file. Great.”
but when expectations fail to be able to easily see what the actual vs expectation is.
There was a problem hiding this comment.
Personal opinion. What I usually do is asking myself: "Does having the result inline break the readability of the test? ". For example, if the result of the test is a huge json (e.g. 100+lines) and there are a lot of these jsons, i would put the jsons in a different file; if the result is about 5 lines, i would put them inline or in the same file.
There was a problem hiding this comment.
I will put them inside each function then
hellohuanlin
added
the
autosubmit
hellohuanlin
force-pushed
the
quick_actions_refactor_with_100_coverage
branch
from
8ed7a12 to
9bc4b46
Compare
3 participants
FLTShortcutStateManagerwhich manages a list of all shortcut items. TheFLTQuickActionsPluginclass does not directly depend on[UIApplication sharedApplication].shortcutItemsanymore.launchingShortcutType.This refactor allows us to reach 100% unit test coverage without exposing private APIs.
List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#108750
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.