Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS: perform actions on UIPickerView #605

Merged
merged 21 commits into from
Mar 26, 2018
Merged

Conversation

DmitryPonomarenko
Copy link
Contributor

Add test screen with pickerView(iOS)
Add tests to e2e folder for this pickerView

});

it('check and scroll datePicker', async () => {
await expect(element(by.type('UIPickerView'))).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why by type and not by id? Does it not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UIPickerView have not accessibility ID, testID = {'CustomDatePicker'} need to be deleted

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a problem. If user has more than one pickers, the API will break

Copy link
Contributor

Choose a reason for hiding this comment

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

UIPickerView is a UIView, so it can have an accessibility ID. Are you seeing that UIDatePicker does not forward that ID?

Copy link
Contributor Author

@DmitryPonomarenko DmitryPonomarenko Mar 6, 2018

Choose a reason for hiding this comment

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

I checked that UI element via AccessebilityInspector (from Xcode Instruments), and now I found only one way to get element by type

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks

Choose a reason for hiding this comment

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

It won't work on android

Copy link
Contributor

Choose a reason for hiding this comment

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

We know. We want to hone down iOS support first, then we will implement Android as well.

@@ -491,10 +519,14 @@
39A34C5D1E30ED3600BEBB59 /* libcxxreact.a */,
39A34C5F1E30ED3600BEBB59 /* libjschelpers.a */,
39A34C611E30ED3600BEBB59 /* libjschelpers.a */,
D58447B0204E9C6000248543 /* libjsinspector.a */,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this? What will happen if old RN is used? In CI, we have an RN matrix that changes RN versions on the fly. Currently we are testing RN 51 and 53.3

render() {
return (
<View style={{flex: 1, paddingTop: 20, justifyContent: 'center', alignItems: 'center'}}>
<DatePickerIOS testID = {'CustomDatePicker'}
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is testID why find by type in the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UIPickerView have not accessibility ID, testID = {'CustomDatePicker'} need to be deleted

@LeoNatan
Copy link
Contributor

LeoNatan commented Mar 6, 2018

In the future, please do not open new PRs, just push in to previous.

From the previous PR discussion:

Android implementation needs to have a different API. We can not use same API for both, Android has only date or time pickers, iOS has date, time and datetime pickers, therefore:
iOS API should use setColumnToValue
Android API should use setToDate(y, m, d) and setToTime(h, m)

Android resources:
DatePicker
TimePicker
Espresso API:
PickerActions
Example

@LeoNatan
Copy link
Contributor

LeoNatan commented Mar 6, 2018

You are missing the docs/APIRef.ActionsOnElement.md information in this PR. Previous PR had the update there.

it('check and scroll datePicker', async () => {
await expect(element(by.type('UIPickerView'))).toBeVisible();
await expect(element(by.type('UIPickerView')).setColumnToValue(1,"6"));
await expect(element(by.type('UIPickerView')).setColumnToValue(2,"34"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Expect should not be on actions.
You should have a label and see that the correct value is shown there.

Should be something like

await element(by.type('UIPickerView')).setColumnToValue(2,"34");
await expect(element(by.id('time'))).toHaveText('6:34');

@timsawtell-sportsbet
Copy link

timsawtell-sportsbet commented Mar 6, 2018

I was working on a PR of my own to fix this issue and should have checked here first! Very keen to see this enhancement get in, as it's a blocker for our tests at the moment.

Would it be possible to get this merged with support for iOS only? I notice the implementation for Android is missing, and I myself haven't attempted to implement the Android version because we're not using it. Surely motivated Android devs could implement the [equivalent to] GreyAction for Android should they have a desire to get it working. crosses fingers

@LeoNatan
Copy link
Contributor

LeoNatan commented Mar 7, 2018

@timsawtell-sportsbet We discusses this internally and finallly decided we will merge it before the Android support lands. Once the PR is satisfactory (very soon), we will merge.

@LeoNatan LeoNatan mentioned this pull request Mar 7, 2018
@LeoNatan
Copy link
Contributor

Why is this stale for so long? @DmitryPonomarenko
Now there are conflicts in the branch. Please merge mast first and then make the changes as needed.

# Conflicts:
#	detox/test/ios/example.xcodeproj/project.pbxproj
#	detox/test/ios/example/Info.plist
#	detox/test/src/Screens/index.js
#	detox/test/src/app.js
Copy link
Contributor

@LeoNatan LeoNatan left a comment

Choose a reason for hiding this comment

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

The tests are failing. Please check why and fix where needed.

"name": "generation",
Copy link
Contributor

Choose a reason for hiding this comment

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

What changed in this file? If nothing, please revert this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

This still remains open. Why did the file change?

@@ -107,3 +108,13 @@ await element(by.id('scrollView')).swipe('down');
await element(by.id('scrollView')).swipe('down', 'fast');
await element(by.id('scrollView')).swipe('down', 'fast', 0.5);
```
### `setColumnToValue(column,value)`
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be explicitly marked as iOS only in the documentation.

@@ -1145,7 +1201,7 @@
"-ObjC",
"-lc++",
);
PRODUCT_BUNDLE_IDENTIFIER = "org.reactjs.native.example.$(PRODUCT_NAME:rfc1034identifier)";
PRODUCT_BUNDLE_IDENTIFIER = org.reactjs.native.example.example;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change necessary?

});

it('check and scroll datePicker', async () => {
await expect(element(by.type('UIPickerView'))).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should be wrapped with if(iOS).

it('check and scroll datePicker', async () => {
await expect(element(by.type('UIPickerView'))).toBeVisible();
await element(by.type('UIPickerView')).setColumnToValue(1,"6");
await element(by.type('UIPickerView')).setColumnToValue(2,"34");
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now, this test checks nothing. There should be a label in the screen, and on date picker change value, it should update. You should then assert that the label has the correct value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking label with text will be always true, because this label always on date picker, I think right solution is checking property of datePicker current value or something like that, now I work in this direction, add this functionality asap

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand. The label is updated with the value set in the picker. So if you select 6:41, it won't say "6:34" in the label. If you assert that label.text == "6:34", you will test that the picker really changed value. That's what we want to test in the testing suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my example if element (UIPickerView) can't find variable with current column or value test will be failed

Copy link
Contributor

Choose a reason for hiding this comment

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

But that is not what we test in the suite. We test that the native system behaves as expected following Detox changes.

@LeoNatan
Copy link
Contributor

As we discussed, also please test if Earl Grey has API to assert on the picker value at column.

await element(by.text('DatePicker')).tap();
});

it('datePicker and dateLabel should be visible', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessary.

@@ -1137,15 +1137,15 @@
buildSettings = {
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
DEAD_CODE_STRIPPING = NO;
DEVELOPMENT_TEAM = "";
DEVELOPMENT_TEAM = S3GLW74Y8N;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why these changes are going in?
Please revert it. The entire project,pbxproj file changes seem to me like something that should not have been committed.

"name": "generation",
Copy link
Contributor

Choose a reason for hiding this comment

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

This still remains open. Why did the file change?

@LeoNatan LeoNatan merged commit 702597b into master Mar 26, 2018
@LeoNatan LeoNatan deleted the FunctionalityForTestingPickerIOS branch March 26, 2018 12:41
@rotemmiz rotemmiz changed the title Functionality for testing picker ios IOS: perform actions on UIPickerView Mar 26, 2018
@rotemmiz rotemmiz changed the title IOS: perform actions on UIPickerView iOS: perform actions on UIPickerView Mar 26, 2018
yershalom pushed a commit that referenced this pull request Apr 4, 2018
Implement functionality for changing the column values of pickers on IOS
@wix wix locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants