feat: Add flutter integration driver commands and tests #1022
Conversation
|
Could you add tests in unit test as possible instead of functional test? As a client, what http request should be sent is important but it does not need to test driver side. Driver's response can be a mock. Existing functional tests include tests for appium project's drivers themselves. They would be removed as not necessary for client tests. |
|
Could you use |
|
I left some comments.
Thank you for the contribution, btw :) |
| None: | ||
| """ | ||
| if isinstance(locator, WebElement): | ||
| opts = {'element': locator, 'timeout': time_out} |
There was a problem hiding this comment.
should we actually pass the element object itself or its identifier?
There was a problem hiding this comment.
it handled within appium-flutter-server https://github.com/AppiumTestDistribution/appium-flutter-server/blob/0019f6ebf242c78c211872d8c64744f3582bc8b7/server/lib/src/handler/wait/base_wait.dart#L21
There was a problem hiding this comment.
I am not sure it is going to work like that. Could you please point me to a passing e2e test that verifies this approach?
There was a problem hiding this comment.
here is the e2e test https://github.com/appium/python-client/pull/1022/files#diff-025756cb750b47a876194b14a36ff2ebf8236b98a581eb1c7c1249873425a87bR40 and the same has been implemented in java client too https://github.com/appium/java-client/blob/9b3194e29857b6204f51d7f198d3583d2d904e50/src/main/java/io/appium/java_client/flutter/commands/WaitParameter.java#L30
There was a problem hiding this comment.
thanks. I mean I would like to check the github actions log where the test has passed
There was a problem hiding this comment.
Currently all flutter tests fail with https://github.com/appium/python-client/actions/runs/10966892515/job/30463953515?pr=1022#step:17:76
You may verify they work as expected by running tests in the local fork
updated |
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| from typing import Dict |
There was a problem hiding this comment.
please apply black and isort formatting
There was a problem hiding this comment.
thanks! Applied for all files
| UP = 'up' | ||
| DOWN = 'down' | ||
|
|
||
| def as_string(self): |
There was a problem hiding this comment.
this method is not really needed. You may use .value
There was a problem hiding this comment.
was the change committed?
appium/webdriver/flutter_finder.py
Outdated
| def to_dict(self) -> dict: | ||
| return { 'using': self.using, 'value': self.value} | ||
|
|
||
| def as_args(self) -> str: |
| from appium.webdriver.webelement import WebElement | ||
|
|
||
|
|
||
| class FlutterCommand: |
There was a problem hiding this comment.
How about renaming this class to FlutterExtensions?
There was a problem hiding this comment.
FlutterExtensions sounds more generic and doesn't immediately convey that it's about commands or actions.
I would rather like to stick with same name or FlutterActions. Please suggest
There was a problem hiding this comment.
That's just a proposal. If you like Command more then let it be
| scrollView (str): The view of the scroll. | ||
| delta (int): delta for the scroll | ||
| maxScrolls (int): Max times to scroll | ||
| settleBetweenScrollsTimeout (int): settle timeout |
There was a problem hiding this comment.
Please mention explicitly the time unit used there (I assume milliseconds) and default values if not provided
There was a problem hiding this comment.
fixed and added the default values
There was a problem hiding this comment.
hm, I still cannot find neither for KeywordArgs
| Scrolls until the specified element becomes visible. | ||
| Args: | ||
| scroll_to (FlutterFinder): The Flutter element to scroll to. |
There was a problem hiding this comment.
there is no need to repeat types in docstrings
There was a problem hiding this comment.
are you suggesting me to remove (FlutterFinder)?
There was a problem hiding this comment.
yes, do it for all args where the time is set explicitly upon declaration
| @@ -25,3 +25,8 @@ class AppiumBy(By): | |||
| ACCESSIBILITY_ID = 'accessibility id' | |||
| IMAGE = '-image' | |||
| CUSTOM = '-custom' | |||
| FLUTTER_INTEGRATION_SEMANTICS_LABEL = '-flutter semantics label' | |||
There was a problem hiding this comment.
👍
Can you leave a link as a comment for those as not in appium org repo for future maintenance?
There was a problem hiding this comment.
Thanks. Could you please clarify link I should add in comment .
There was a problem hiding this comment.
I think the driver project's GitHub repo is good
| else: | ||
| opts['locator'] = locator.to_dict() | ||
| if time_out is not None: | ||
| opts['timeout'] = time_out |
There was a problem hiding this comment.
does the downstream API also accept float seconds or should we provide integer milliseconds instead?
| None: | ||
| """ | ||
| opts: Dict[str, Union[WebElement, Dict[str, str], float]] = {} | ||
| if isinstance(locator, WebElement): |
There was a problem hiding this comment.
this condition is duplicated multiple times. Please extract it to a function
Addressing AppiumTestDistribution/appium-flutter-integration-driver#52