iOS: Add pinch actions #1104

sraikimaxime · 2018-12-30T18:45:01Z

This is a small change
This change has been discussed in issue #<1105> and the solution has been agreed upon with maintainers. - This is wip, the solution has not been agreed on and I haven't followed the contribution guide yet, sorry. My work started after seeing issues #<589> and #<250>.

Description:
This is the first step of the implementation of tools allowing to manipulate react-native-maps MapView. I'm opening this PR to have the earliest challenge on how I'm going forward.

##iOS

I've been looking to how EarlGrey proposes to manipulate maps but as far as my understanding goes, it only allows to swipe on it and pinch on it to handle the zoom. Therefore I've been testing to connect expect with earl grey's actionForPinchFastInDirectionWithAngle and actionForPinchSlowInDirectionWithAngle And managed to manipulate the map like in the gif I add to this description.

So far I've been able to zoom in, zoom out and move around in the react native maps thanks to these actions.

##Android
Haven't started yet :)

sraikimaxime · 2018-12-30T21:59:47Z

Updating description as I implemented the functions inside the generation process.

Still have a few things to handle:

Fix the "double" type as it appears it fails if it is not an NSNumber I'll try to remap it but I'm not sure if it is a really long term solution.
Reach 100% coverage to pass the CI

I also added a lot of type into "supported types" but I don't really understand what it is supposed to mean when we declare types are "supported" should I do something more inside other parts of the code ?

sraikimaxime · 2018-12-30T22:10:06Z

I did the remapping from double to NSNumber and the generated GreyActions are working OK but I'm still not sure of how good a practice it is :)

sraikimaxime · 2019-01-01T15:54:05Z

I realized I had generated a bunch of functions I didn't really know about by adding their arguments type to the supported types. Could you tell me more about what it does mean to add something in there, what kind of check am I supposed to do on variables before "supporting" them ? I need to support "double" but it also adds another action along with mines.

I also don't know what tests to write to make the coverage up to 100% again, could you please give me a tip ? :)

rotemmiz · 2019-01-02T07:46:51Z

Seems like you managed with unit tests.

I also saw you removed many types from iOS generator adapter, any reason for that?

rotemmiz · 2019-01-02T07:47:45Z

this PR is great! Its a very welcome addition. thanks!

rotemmiz · 2019-01-02T07:50:29Z

Making it 100% safe to merge will require addition of new E2E tests that will actually test this new API.
This test needs to have no new dependencies on the test project, as we want to keep it clean from external noise.

sraikimaxime · 2019-01-02T09:35:32Z

@rotemmiz
"I also saw you removed many types from iOS generator adapter, any reason for that?"

Yes, it looked to me that they were duplicates in the list :)

"Making it 100% safe to merge will require addition of new E2E tests that will actually test this new API.
This test needs to have no new dependencies on the test project, as we want to keep it clean from external noise."

I was thinking about it :) plan to work on a e2e test this week end !
Don't you think it would be interesting to create a blank project with a map ?

I still have a question: What is the aim of the the supported types array? Am I supposed to test some stuff before adding "double" in it ? I Also noted that the generator fails to generate a few functions because their type is not supported I'd love to remove these warnings but I don't want to manipulate these "supported types" before understanding what they mean :)

rotemmiz · 2019-01-02T09:47:12Z

Let's add @DanielMSchmidt to the conversation

sraikimaxime · 2019-01-03T14:30:26Z

Hello @DanielMSchmidt I'm planning on working on e2e tests this week end and would love to have your feedback before I start implementing a e2e project with a react native maps as e2e test, what do you think ?

Also I still don't really understand what is the aim of the "supportedTypes" array, if you know more about it i'd love to read from you :)

Thank you for your time and help anyway.

DanielMSchmidt · 2019-01-03T15:51:47Z

generation/core/generator.js

@@ -70,6 +70,9 @@ module.exports = function getGenerator({
      if (methodArg === null) {
        console.error(method);
      }
+      if (!supportedTypes.includes(methodArg.type)) {


I think we have a separate map for this, which gives you the methods that could not be generated for each class due to which types.

Right ! I'll take it off

DanielMSchmidt · 2019-01-03T15:52:25Z

I looked through it, seems good to me. Thank you for your patience 😇

sraikimaxime · 2019-01-03T16:33:41Z

@DanielMSchmidt Thank you very much for your review ! About the two questions I asked above:

What is the use of supported type ? Should I add to supported types the ones that prevent the generator to create functions (several functions are blocked)
Do you think it would be a good idea to create a blank project with a react-native-maps in order to do the e2e test for this action ?

DanielMSchmidt · 2019-01-03T16:43:26Z

What is the use of supported type?

We have a two-way contract: native side - client side. Both sides need to be able to handle the input of a type. We prevent generating methods for the client side that we can not use on the native side.

Do you think it would be a good idea to create a blank project with a react-native-maps in order to do the e2e test for this action

As @rotemmiz already said we would very much prefer a test case without an external dependency

sraikimaxime · 2019-01-06T19:55:48Z

@rotemmiz @DanielMSchmidt. So I did add the ios e2e tests, now looking for android I'll check it these evening !

sraikimaxime · 2019-01-08T18:44:58Z

Hello @rotemmiz @DanielMSchmidt, I'm having trouble fixing the e2e tests on android as some of them look flaky. Here the stressTimeouts first test run three times in a row:

I don't really understand why it would break like this from time to time, do you have any advice ?

sraikimaxime · 2019-01-08T19:01:57Z

I've been investigating and it seems these tests have been switched from :ios: to cross platform in this PR It looks the CI was red and became green on a rerun, I also noticed the artifacts were not updated at the time. I'm re switching them to :ios: only for now, what do you think ?

sraikimaxime · 2019-01-13T20:19:28Z

@rotemmiz @DanielMSchmidt, Hello guys, I've been adding some docs :) Do you see something missing in order to merge this first step ?

In order to continue on the Android implementation, I have been looking at how it is done in Espresso and it looks like most people use UIAutomator in order to perform two swipes to realize the pinch gesture. If it looks ok to you guys I'll start implementing this ! :)

sraikimaxime · 2019-01-15T14:46:55Z

@d4vidi Hello I'm adding you to the conversation as it looks you are pretty active these days :)

sraikimaxime · 2019-01-21T14:00:34Z

up @LeoNatan @rotemmiz @DanielMSchmidt cf My previous message

DanielMSchmidt · 2019-01-22T19:31:47Z

Sorry for the delay @sraikimaxime, I'm going to take a look right now 💪

DanielMSchmidt

I would love to see some API docs for the newly added feature.

But besides this looks like a great PR, thank you very much 🎉

DanielMSchmidt · 2019-01-22T19:37:57Z

docs/Guide.Contributing.md

+1. In `detox/test` project, build the ios project with `npm run build:ios`.
+2. Run all end-to-end tests on iOS with `npm run e2e:ios`.
+3. Update the snapshots with: `npm run verify-artifacts:ios -- -u`.
+4. In `detox/test` project, build the ios project with `npm run build:android`


I think this line was added, did you mean to build the android project with this line?

Suggested change

4. In `detox/test` project, build the ios project with `npm run build:android`

4. In `detox/test` project, build the android project with `npm run build:android`

Totally right, fixing this :)

rotemmiz · 2019-01-23T00:00:20Z

detox/test/e2e/12.animations.test.js

@@ -51,7 +51,7 @@ describe('Animations', () => {
      await expect(element(by.id('UniqueId_AnimationsScreen_afterAnimationText'))).toNotExist();
    });

-    it(`should wait during delays shorter than 1.5s (driver: ${driver})`, async () => {
+    it(`:ios: should wait during delays shorter than 1.5s (driver: ${driver})`, async () => {


Why did you disable tests for Android?

as I said in this comment these tests look flaky? cause looks like coming from here

I'll try to run them from updated master and let you know the result

rotemmiz · 2019-01-23T00:00:57Z

detox/test/e2e/09.stress-timeouts.test.js

@@ -4,7 +4,7 @@ describe('StressTimeouts', () => {
    await element(by.text('Timeouts')).tap();
  });

-  it('should handle a short timeout', async () => {
+  it(':ios: should handle a short timeout', async () => {
    await element(by.id('TimeoutShort')).tap();


Why did you disable tests for Android?

as I said in this comment these tests look flaky? cause looks like coming from here

I'll try to run them from updated master and let you know the result

rotemmiz · 2019-01-23T00:02:33Z

detox/test/scripts/__snapshots__/verify_artifacts_are_not_missing.ios.test.js.snap

@@ -4,10 +4,10 @@ exports[` 1`] = `
 Array [


We stopped testing artifacts file snapshots, no need to change it. PR might need a rebase/merge from master.

Ok removing it + unupdating the doc :)

Still, shouldn't we remove this snapshot in this case ? if so I can do it in this PR :)

rotemmiz · 2019-01-23T00:06:39Z

Thanks for this PR!
There are a few minor issues with it though, mentioned in comments.

sraikimaxime · 2019-01-30T07:41:34Z

@DanielMSchmidt Thank you ! I have added a small doc on APIRef.ActionsOnElement.md is there another place where you'd expect documentation ?

DanielMSchmidt · 2019-01-30T08:52:16Z

seems fine to me

rotemmiz · 2019-01-30T09:30:51Z

hey @sraikimaxime,

The PR looks good and I would like to merge it in, but still seems like artifact snapshot tests are deleted. We don't currently use them, but deleting them shouldn't be a part of this PR.

Please revert their deletion, and we'll merge it in.
Thanks for this PR!

sraikimaxime · 2019-01-30T18:03:43Z

Thank you @rotemmiz ! :) Should I open a new PR to remove the artifacts ?

sraikimaxime requested a review from rotemmiz as a code owner December 30, 2018 18:45

This was referenced Dec 30, 2018

Improve the way we can interract with react-native-maps #1105

Closed

Can't use .scroll() and .swipe() for MapView from react-native-maps #250

Closed

sraikimaxime force-pushed the master branch from a532d83 to 7ea1b71 Compare January 1, 2019 15:48

sraikimaxime requested a review from LeoNatan as a code owner January 1, 2019 20:10

sraikimaxime force-pushed the master branch from c818f43 to 7ba039a Compare January 1, 2019 20:10

DanielMSchmidt reviewed Jan 3, 2019

View reviewed changes

DanielMSchmidt suggested changes Jan 22, 2019

View reviewed changes

rotemmiz requested changes Jan 23, 2019

View reviewed changes

sraikimaxime force-pushed the master branch from dc102d7 to ccc1b2b Compare January 23, 2019 21:28

sraikimaxime added 15 commits January 30, 2019 08:38

Create the pinch actions and link it to expect

542a1ea

Add pinch direction sanitizer to generator

0584c51

Remove duplicate supportedTypes

3f88f0b

Add grey pinch sanitizer to snapshot test

b46541d

Add double to supported types and remap it to NSNumber

0373851

Add sanitizing test

3ec427a

Unit test pinch action

106f883

Add e2e test for pinch on iOS

cf2e989

Remove duplicate console logger for unsupported types

2039886

Fix ios artifacts

2596eea

Restrict flaky test on android to ios only

1304a52

Add docs on the pinch action

8160bfc

Fixing doc

cc3be91

Remove useless artifacts tests

86cfa2c

Fixing steps numbers on e2e tests modification

0a6b692

Re enable tests on android

eabcb6c

sraikimaxime force-pushed the master branch from ccc1b2b to eabcb6c Compare January 30, 2019 08:07

sraikimaxime force-pushed the master branch 2 times, most recently from af41b6a to 7824c14 Compare January 30, 2019 12:34

Add artifacts snapshots and android tests back

620ae42

sraikimaxime force-pushed the master branch from 7824c14 to 620ae42 Compare January 30, 2019 13:59

rotemmiz approved these changes Jan 30, 2019

View reviewed changes

rotemmiz changed the title ~~Create the pinch actions and link it to expect~~ iOS: Add pinch actions Jan 30, 2019

rotemmiz merged commit 90cbc13 into wix:master Jan 30, 2019

lock bot locked as resolved and limited conversation to collaborators Feb 2, 2019

	4. In `detox/test` project, build the ios project with `npm run build:android`
	4. In `detox/test` project, build the android project with `npm run build:android`

iOS: Add pinch actions #1104

iOS: Add pinch actions #1104

Conversation

sraikimaxime commented Dec 30, 2018 • edited Loading

sraikimaxime commented Dec 30, 2018 • edited Loading

sraikimaxime commented Dec 30, 2018

sraikimaxime commented Jan 1, 2019 • edited Loading

rotemmiz commented Jan 2, 2019

rotemmiz commented Jan 2, 2019

rotemmiz commented Jan 2, 2019

sraikimaxime commented Jan 2, 2019 • edited Loading

rotemmiz commented Jan 2, 2019

sraikimaxime commented Jan 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielMSchmidt commented Jan 3, 2019

sraikimaxime commented Jan 3, 2019

DanielMSchmidt commented Jan 3, 2019

sraikimaxime commented Jan 6, 2019 • edited Loading

sraikimaxime commented Jan 8, 2019

sraikimaxime commented Jan 8, 2019

sraikimaxime commented Jan 13, 2019

sraikimaxime commented Jan 15, 2019

sraikimaxime commented Jan 21, 2019

DanielMSchmidt commented Jan 22, 2019

DanielMSchmidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rotemmiz commented Jan 23, 2019

sraikimaxime commented Jan 30, 2019

DanielMSchmidt commented Jan 30, 2019

rotemmiz commented Jan 30, 2019

sraikimaxime commented Jan 30, 2019

sraikimaxime commented Dec 30, 2018 •

edited

Loading

sraikimaxime commented Dec 30, 2018 •

edited

Loading

sraikimaxime commented Jan 1, 2019 •

edited

Loading

sraikimaxime commented Jan 2, 2019 •

edited

Loading

sraikimaxime commented Jan 6, 2019 •

edited

Loading