Add generation for GREYInteraction #564

DanielMSchmidt · 2018-02-06T21:43:50Z

This PR moves most iOS calls over to generated code (detox_selectElementWithMatcher is the only missing one and it's a bit of a special case.

rotemmiz · 2018-03-06T18:59:01Z

Is this still a WIP?

DanielMSchmidt · 2018-03-06T19:06:06Z

Yes, but close to being finished

DanielMSchmidt · 2018-03-11T14:50:16Z

I now rebased this PR. The original goal was more ambitious, but I think incremental steps forward are a nicer way to work together 🙈 I think this might help with #618 maybe only by us having a conversation about what these instance methods mean for us in detox 👍

rotemmiz · 2018-03-11T18:30:47Z

generation/__tests__/ios.js

@@ -333,6 +333,28 @@ describe("iOS generation", () => {
 		});
 	});

+	describe("instance methods", () => {


Those tabs look awful in github :(

They look great. Don’t switch to spaces.

rotemmiz · 2018-03-11T18:34:22Z

generation/core/generator.js

@@ -7,13 +7,14 @@ const fs = require("fs");

 const { methodNameToSnakeCase } = require("../helpers");
 let globalFunctionUsage = {};
-module.exports = function({
+module.exports = function ({


Could you please make it a bit more readable by creating a named function and exporting it, instead of exporting an anonymous one. Sorry for nitpicking, I just can't get used to this javascript jungle magic. :(

sure, what would be a good name for a function that returns a code generator?

getGenerator ?

rotemmiz · 2018-03-11T18:36:12Z

generation/core/generator.js

@@ -81,13 +89,22 @@ module.exports = function({
 	}

 	function createMethod(classJson, json) {
+		// For us this means that we don't set the class type, but let it be set from the outside
+		// This is an assumption based on our current experience with EarlGrey, we might need to rework this at some point
+		const isTranslucent = !json.static;


what does translucent mean in this context ?

It means that we don't wrap the content as a class but add another argument called element to the function list, which is taken as the element to work on. From the return statements perspective the translucent naming makes sense, but we could also call it isInstanceMethod, might make more sense in this context

Not sure I got it, can you elaborate?

Sorry, pressed enter too early, does this make more sense?

rotemmiz · 2018-03-11T18:37:31Z

generation/core/generator.js

@@ -112,7 +129,7 @@ module.exports = function({
 		return json;
 	}

-	function createMethodBody(classJson, json) {
+	function createMethodBody(classJson, json, isTranslucent) {


try not to pass booleans in function params, it's very confusing.
https://martinfowler.com/bliki/FlagArgument.html

Instead, just create two functions with clear naming

I moved this part down into the functions

rotemmiz · 2018-03-11T18:42:22Z

generation/core/generator.js

+							t.identifier("type"),
+							t.stringLiteral(addArgumentTypeSanitizer(arg))
+						),
+						t.objectProperty(


Is this formatting the work of prettier ? It's very weird,

Prettier being prettier 🤷‍♂️

rotemmiz · 2018-03-11T18:43:46Z

generation/core/generator.js

@@ -215,7 +243,7 @@ module.exports = function({
 			: typeCheckCreator(json);
 	}

-	return function(files) {
+	return function (files) {


Again, javascript voodoo. I'm too old for this magic. ;(

What is going on here?

The generator is returning a function to be invoked with files to generate the source code. The problem was that iOS and Android have a similar abstraction, so the easiest way to solve this without having to rewrite a lot was to wrap it into an anonymous functions with all the static values previously set being configurable and returning the old main logic as a function.

rotemmiz · 2018-03-11T18:45:10Z

generation/package.json

@@ -23,7 +23,6 @@
 		"jest": "^20.0.4",
 		"lerna": "2.0.0-rc.4",


You don't need lerna here, it can be safely deleted

rotemmiz · 2018-03-11T18:47:28Z

Of course this needs to pass e2e before merge (from travis log)

     Error: Error: target is invalid
      at Client.execute (/Users/travis/build/wix/detox/detox/src/client/Client.js:75:13)
      at <anonymous>

DanielMSchmidt · 2018-03-12T21:27:47Z

@rotemmiz I refactored the things you mentioned and got a working version up (let's see if CI agrees). I could only finish one of the calls, but left TODO items on the others. They "only" need a bit of fine tuning, but I thought this progress might already be helpful for you, especially in regards of #618

rotemmiz · 2018-03-12T21:30:38Z

Awesome! Will we be able to generate non static invocations now ? Can't see an example in the new generated code, can you please point me to it ?

DanielMSchmidt · 2018-03-12T22:20:36Z

detox/src/ios/expect.js

-    this._call = invoke.call(element._call, 'performAction:', action._call);
-    // TODO: move this.execute() here from the caller
+
+    this._call = GreyInteraction.performAction(callThunk(element), callThunk(action));


@rotemmiz this is currently the only successful invocation of an instance method, I need to find out why the others fail, but I guess it's a call side problem

Is this resolved ?

rotemmiz · 2018-03-21T06:54:29Z

Hey @DanielMSchmidt , anything to be done on your side ?

DanielMSchmidt · 2018-03-21T07:15:32Z

Not yet, trying to get to it next weekend

DanielMSchmidt · 2018-03-27T20:51:07Z

@rotemmiz This is done now 👍 Let's get it into master ;)

Addressed changes

DanielMSchmidt · 2018-04-12T20:15:52Z

@rotemmiz ping, could you review this PR? 🙏 😇

rotemmiz · 2018-04-16T12:19:19Z

generation/core/generator.js

 			t.blockStatement(createMethodBody(classJson, json)),
 			false,
-			json.static
+			true


will this mean every method is static now ?

Then why passing this boolean anyway ? why not get rid of it ?

https://martinfowler.com/bliki/FlagArgument.html

This is a method of babel-types, I could write a wrapper like createStaticClassMethod, would that be more readable for you?

Every method is static, I choose to deal with instance methods a little different, I just let the target be passed in as the first argument. My point for this is that we have the class-like behavior more on the caller-side of this function and I don't want to duplicate it in any way. Also, the calls are more similar now than they would have been otherwise. If we want to change this somewhen I would propose we rethink this at a point in time when we get rid of these wrapper classes and access the generated code "directly", like in this proposal: #563

rotemmiz · 2018-04-16T12:19:50Z

generation/core/generator.js

@@ -189,10 +214,12 @@ module.exports = function({
 				t.objectProperty(
 					t.identifier("target"),
 					t.objectExpression([
-						t.objectProperty(t.identifier("type"), t.stringLiteral("Class")),
+						t.objectProperty(t.identifier("type"), t.stringLiteral(json.static ? "Class" : "Invocation")),


rotemmiz · 2018-04-16T12:22:53Z

generation/core/type-checks.js

-	});
+			ARG: t.identifier(name)
+		});
+const isGreyAction = ({ name }) =>


What do you think about just defining functions instead of assigning functions to variables ?

function isGreyAction({name}) { }

I like the idea 👍

DanielMSchmidt · 2018-04-17T05:39:22Z

@rotemmiz Addressed your comments / replied. Thank you for taking the time to review <3

DanielMSchmidt · 2018-04-19T06:41:39Z

Fun fact: This PR moves us from 47 to 40 open .call invocations

rotemmiz · 2018-04-19T20:33:52Z

💪

rotemmiz · 2018-04-19T20:53:01Z

@DanielMSchmidt , I'm getting a weird behaviour when running ci.ios.sh locally.
generation script generates something a bit different and breaks tests. Can you please take a look ?

rotemmiz · 2018-04-19T21:01:12Z

I now get it, I switched to EarlGrey 1.13, there is some breakage, not sure what.

rotemmiz · 2018-04-19T21:25:17Z

@LeoNatan, NS_REFINED_FOR_SWIFT macro was added to a few of EarlGrey's functions.
What should we do if we want to invoke this code through DTXMethodInvocation? should we just ignore it in the method signature ?

LeoNatan · 2018-04-19T21:35:04Z

Refined means there is a better alternative for Swift. It has no influence on ObjC.

rotemmiz · 2018-04-19T21:41:19Z

So
method: "performAction:"
not
method: "performAction:NS_REFINED_FOR_SWIFT"

right?

LeoNatan · 2018-04-19T21:48:16Z

Yes, of course.

It's an attribute.
# define CF_REFINED_FOR_SWIFT __attribute__((swift_private))

DanielMSchmidt force-pushed the generation/ios-element-interaction branch 2 times, most recently from 138e75a to b206d1a Compare February 10, 2018 10:58

rotemmiz changed the title ~~Add generation for GREYInteraction~~ [WIP] Add generation for GREYInteraction Mar 6, 2018

DanielMSchmidt force-pushed the generation/ios-element-interaction branch from b206d1a to b8b2f2e Compare March 11, 2018 14:51

DanielMSchmidt requested review from LeoNatan and rotemmiz as code owners March 11, 2018 14:51

DanielMSchmidt changed the title ~~[WIP] Add generation for GREYInteraction~~ Add generation for GREYInteraction Mar 11, 2018

rotemmiz reviewed Mar 11, 2018

View reviewed changes

rotemmiz previously requested changes Mar 11, 2018

View reviewed changes

DanielMSchmidt force-pushed the generation/ios-element-interaction branch 4 times, most recently from a9f2559 to eeba8fa Compare March 12, 2018 21:26

DanielMSchmidt commented Mar 12, 2018

View reviewed changes

DanielMSchmidt force-pushed the generation/ios-element-interaction branch from eeba8fa to ba5df91 Compare March 14, 2018 07:44

DanielMSchmidt force-pushed the generation/ios-element-interaction branch from ba5df91 to 4d35bcc Compare March 27, 2018 20:45

Move manual calls on iOS to generated code

8b6d9d9

DanielMSchmidt force-pushed the generation/ios-element-interaction branch from 4d35bcc to 8b6d9d9 Compare March 27, 2018 20:50

rotemmiz requested changes Apr 16, 2018

View reviewed changes

formatting and functions instead of arrow functions

7e7dadf

rotemmiz approved these changes Apr 19, 2018

View reviewed changes

rotemmiz merged commit 479a1b4 into master Apr 19, 2018

rotemmiz deleted the generation/ios-element-interaction branch April 19, 2018 20:32

wix locked and limited conversation to collaborators Jul 23, 2018

Add generation for GREYInteraction #564

Add generation for GREYInteraction #564

Conversation

DanielMSchmidt commented Feb 6, 2018 • edited Loading

rotemmiz commented Mar 6, 2018

DanielMSchmidt commented Mar 6, 2018

DanielMSchmidt commented Mar 11, 2018

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

DanielMSchmidt Mar 12, 2018 • edited Loading

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

Choose a reason for hiding this comment

rotemmiz commented Mar 11, 2018

DanielMSchmidt commented Mar 12, 2018

rotemmiz commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rotemmiz commented Mar 21, 2018

DanielMSchmidt commented Mar 21, 2018

DanielMSchmidt commented Mar 27, 2018

DanielMSchmidt commented Apr 12, 2018

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

DanielMSchmidt commented Apr 17, 2018

DanielMSchmidt commented Apr 19, 2018

rotemmiz commented Apr 19, 2018

rotemmiz commented Apr 19, 2018

rotemmiz commented Apr 19, 2018

rotemmiz commented Apr 19, 2018

LeoNatan commented Apr 19, 2018

rotemmiz commented Apr 19, 2018

LeoNatan commented Apr 19, 2018

DanielMSchmidt commented Feb 6, 2018 •

edited

Loading

DanielMSchmidt Mar 12, 2018 •

edited

Loading