Android: Move expect.js to generated code #746

DanielMSchmidt · 2018-05-19T19:04:23Z

Closes #726

rotemmiz · 2018-05-20T19:44:36Z

getting multiple errors in CI

m should be an instance of Matcher, got "[object Object]", it appears that it has a wrong class name: "Object"

maybe add a toString to that object
unit tests are failing...

DanielMSchmidt · 2018-05-20T20:56:36Z

Will check that out 👍 worked on my machine TM

noomorph · 2018-07-29T14:12:39Z

E2E tests don't pass on Android, almost everything fails with the same error:

  ● Async and Callbacks › should handle async await

    Error: Unhandled arg typeViewInteraction

      71 |     // when this test run fails, we want a stack trace from up here where the
      72 |     // $callee is still available, and not inside the catch block where it isn't
    > 73 |     const potentialError = new Error()
      74 | 
      75 |     try {
      76 |       await this.sendAction(new actions.Invoke(invocation));
      
      at Client.execute (../../src/client/Client.js:73:28)
      at InvocationManager.execute (../../src/invoke.js:11:33)
      at MatcherAssertionInteraction.execute (../../src/android/expect.js:112:29)
      at ExpectElement.toBeVisible (../../src/android/expect.js:244:87)
      at Object.it (10.async-and-callbacks.test.js:17:47)

@DanielMSchmidt , could you take a look please? Try out locally and report what you're getting.
Thanks in advance!

noomorph · 2018-07-29T14:13:53Z

And I confirm that still, I can see the console.error stuff @rotemmiz reported above:

  console.error ../src/android/espressoapi/DetoxAssertion.js:21
    m should be an instance of Matcher, got "[object Object]", it appears that it has a wrong class name: "Object"

It was printed 3-4 times during the test run, is it okay?

DanielMSchmidt · 2018-07-29T15:15:13Z

@noomorph What @rotemmiz saw was an error being thrown, we now have the console.errors, because I could not find the root cause of the check going wrong

DanielMSchmidt · 2018-08-08T20:41:25Z

@noomorph Just checked to be sure and the E2E tests are green and fine 👍

DanielMSchmidt · 2018-08-08T21:45:27Z

@noomorph I added the logger to the generated code, please take a look and merge at will 👍

noomorph

@DanielMSchmidt, thanks much! In regards to the codebase, it looks mostly fine except for a few minutiae I listed in the review.

noomorph · 2018-08-09T07:57:32Z

generation/__tests__/android.js

@@ -2,6 +2,11 @@ const fs = require('fs');
 const remove = require('remove');
 const androidGenerator = require('../adapters/android');

+const mockErrorLog = jest.fn();
+jest.mock("../../detox/src/utils/logger", () => ({


No need to sophisticate an already sophisticated thing. The logger has already a mocked version __mocks__/logger.js, so you can just use it as is.

I removed my own mock and used the one defined in __mocks__/logger.js Unfortunately we have two separate jest installations/contexts, this means I need to do a bit of glue code, but no biggie. I wonder what might happen if we would make more use of a central jest installation with multiple configs with the jest multi-project runners

Might give it a try if I have some spare time 🤔

noomorph · 2018-08-09T07:59:17Z

generation/__tests__/android.js

        expect(() => {
          ExampleClass.matcherForAnd('I am a string', 'I am one too');
        }).not.toThrow();


-        expect(spy).toHaveBeenCalled();
-        spy.mockRestore();
+        expect(mockErrorLog).toHaveBeenCalled();


Correspondingly, you can use expect(logger.error).toHaveBeenCalledWith(expect.any(String))

🤷‍♂️ I don't really see a point in having the expect.any(String), but yeah it might sometime in the future catch something, so I'll add it :)

noomorph · 2018-08-09T07:59:44Z

generation/__tests__/android.js

        class AnotherClass {}
        const x = new AnotherClass();

        expect(() => {
          ExampleClass.matcherForAnd(x, x);
        }).not.toThrow();

-        expect(spy).toHaveBeenCalled();
-        spy.mockRestore();
+        expect(mockErrorLog).toHaveBeenCalled();


Correspondingly, you can use expect(logger.error).toHaveBeenCalledWith(expect.any(String))

noomorph · 2018-08-09T08:01:05Z

generation/core/generator.js

+      const outputPath = pathFragments.join("/")
+      const absoluteUtilsPath = path.resolve('../detox/src/utils');
+      const relativeUtilsPath = path.relative(outputPath, absoluteUtilsPath);
+      const logImport = `const log = require('${relativeUtilsPath}/logger');`;


Change to const log = require('${relativeUtilsPath}/logger').child({ __filename });, so we can track from which files the log messages come.

Also, could you extract this logic to a separate function, similar to a function createTypeCheck(...) above? I see that generator function looks already loong enough, why make it even bigger? :-)

Sure, no problem 👍 I think there are some parts I would like to slim down a bit and structure better, will go on with this soonish. My first goal was to get it working,so that I can see every edge case I ran into and make a better layer of abstraction later on ;)

DanielMSchmidt · 2018-08-11T14:32:48Z

@noomorph Could you take another look 🙏

noomorph · 2018-08-11T14:42:19Z

Looks good the way it is already. If builds pass, more or less, we can try to merge.

P.S. Out of curiosity, why do you do log.error. mockRestore manually?

DanielMSchmidt · 2018-08-11T15:04:25Z

@noomorph are the reset by default?

DanielMSchmidt · 2018-09-02T10:57:52Z

@noomorph Fixed that (the is defined check had a bug so it didn't occur previously, but that is fixed now) and the tests run through 👍

noomorph · 2018-09-05T16:00:33Z

detox/src/android/espressoapi/ViewActions.js

@@ -40,6 +65,24 @@ class ViewActions {
    };
  }

+  static click(rollbackAction) {
+    if (!rollbackAction) {


@DanielMSchmidt , I see unit test failing on this (src/android/expect.js). Is this a reason why you hesitated to add isThruthy check?

I will take another look at the unit tests 👍 No, I hesitated because I don't see so much value 🤷‍♂️

I should have hesitated because of this. The problem is it is optional and not required and we have no way to check if an argument is required or not. So all checks we do should test if the thing that is passed is what it promises to be if something is passed. If null is passed we won't be able to guard us on this side of the fence. I will do a quick research if there is any hypothetical way for us to know if sth is optional and I will let you know.

noomorph

Unit tests are failing, we cannot merge.

DanielMSchmidt · 2018-09-07T21:18:00Z

@noomorph Unit should be fixed now, got weird android simulator issues locally, let
s see what the CI says 😇

noomorph · 2018-09-08T04:41:46Z

Daniel, looks like it is still broken on Android. At least that's what CI tells.

DanielMSchmidt · 2018-09-08T07:04:13Z

Will take a look 🙈

DanielMSchmidt · 2018-09-08T12:37:03Z

@noomorph Could not reproduce, ran it three times on android and one time on iOS and everything went fine.

DanielMSchmidt · 2018-11-21T16:05:57Z

So, this branch is now up to date 🎉 Locally it worked (before the massive rebasing I just did), so let's see what CI says 👍

rotemmiz · 2018-11-21T18:13:44Z

Looks like the branch is not on par with master, there was an artifacts snapshot test we disabled but still runs here, can you make sure to merge current master to this branch?

DanielMSchmidt · 2018-11-21T18:45:53Z

@rotemmiz That's strange, I rebased it from master before I pushed, so I am a bit confused. I can't see where I enabled it looking through the diff.

DanielMSchmidt · 2018-11-21T19:08:25Z

I tried to see if sth is different on the JSON part send to the WS for the failing test, but they are identical (besides message id). So I think your hint into enabling a disabled functionality seems to be right.

DanielMSchmidt · 2018-11-25T09:11:05Z

This is fixed and green on CI. @noomorph and @rotemmiz please review 😇

rotemmiz

I'm having trouble understanding the overload functions generation. Can you explain about that a bit?

rotemmiz · 2018-11-26T21:04:49Z

detox/src/android/espressoapi/UIDevice.js

-        value: metaState
-      }]
-    };
+    function pressKeyCode1(keyCode) {


What happened here?
I guess there's problem with Java overloads, did you handle it somewhere else in a different way?
Seems like the original code just has metaState=0 as default.
https://android.googlesource.com/platform/frameworks/testing/+/79693ed/uiautomator/library/src/com/android/uiautomator/core/UiDevice.java#285

The easiest way to handle overloading is to just check for the number of arguments. There are for sure better, more reliable ways, but this was the fastest. (I have checks in place that make sure we get some console output if we mess up here)

rotemmiz · 2018-11-26T21:05:14Z

detox/src/android/espressoapi/UIDevice.js

+      };
+    }
+
+    if (arguments.length === 1) {


is this generated code?

rotemmiz · 2018-11-26T21:08:40Z

detox/src/android/espressoapi/ViewActions.js

      args: []
    };
  }

+  static click() {
+    function click0() {


Again here, why are there two clicks?
I see only one in UIDevice API

There are two because the method is overloaded.

rotemmiz · 2018-11-26T21:10:14Z

detox/src/ios/earlgreyapi/EarlGrey.js

@@ -0,0 +1,11 @@
+/**


We can probably throw this away, as this class is empty. Everything is in EarlGreyImpl

rotemmiz · 2018-11-26T21:12:20Z

generation/index.js

-		"../detox/src/ios/earlgreyapi/GREYConfiguration.js",
-  "../detox/ios/EarlGrey/EarlGrey/Core/EarlGreyImpl.h":
-    "../detox/src/ios/earlgreyapi/EarlGreyImpl.js"
+  '../detox/ios/EarlGrey/EarlGrey/Action/GREYActions.h': '../detox/src/ios/earlgreyapi/GREYActions.js',


Closes #726

Previously we had manually changed code in the generated sections.

DanielMSchmidt requested a review from rotemmiz as a code owner May 19, 2018 19:04

DanielMSchmidt changed the title ~~Move expect.js to generated code~~ [WIP] Move expect.js to generated code May 22, 2018

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch 2 times, most recently from 8fba90e to 32f10ed Compare May 29, 2018 17:35

DanielMSchmidt changed the title ~~[WIP] Move expect.js to generated code~~ Move expect.js to generated code May 30, 2018

rotemmiz approved these changes Jun 2, 2018

View reviewed changes

DanielMSchmidt changed the title ~~Move expect.js to generated code~~ [WIP] Move expect.js to generated code Jun 3, 2018

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch 3 times, most recently from ff07b22 to e84901e Compare June 10, 2018 21:20

DanielMSchmidt changed the title ~~[WIP] Move expect.js to generated code~~ Move expect.js to generated code Jun 10, 2018

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch 3 times, most recently from 9d1a013 to b89e24f Compare July 12, 2018 20:34

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch from b89e24f to 41f4724 Compare July 29, 2018 13:30

noomorph self-requested a review July 29, 2018 14:26

noomorph added the domain: generation label Aug 7, 2018

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch from 41f4724 to 9db4bc7 Compare August 8, 2018 20:54

noomorph requested changes Aug 9, 2018

View reviewed changes

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch from ac3b7bf to 94adbd3 Compare August 11, 2018 14:28

noomorph approved these changes Sep 5, 2018

View reviewed changes

noomorph reviewed Sep 5, 2018

View reviewed changes

noomorph requested changes Sep 5, 2018

View reviewed changes

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch from 9cb6bc6 to d9e9d00 Compare September 8, 2018 12:38

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch from d9e9d00 to fa808bb Compare October 6, 2018 19:09

rotemmiz changed the title ~~Move expect.js to generated code~~ [WIP] Move expect.js to generated code Oct 21, 2018

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch from b9958f0 to 2f52327 Compare November 21, 2018 16:04

DanielMSchmidt changed the title ~~[WIP] Move expect.js to generated code~~ Move expect.js to generated code Nov 21, 2018

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch 2 times, most recently from 2cce220 to f072bf2 Compare November 25, 2018 08:33

rotemmiz requested changes Nov 26, 2018

View reviewed changes

Daniel Schmidt added 3 commits December 11, 2018 13:41

apply prettier styles to code generation

6f5e3a4

move expect to generated code

9793e53

Closes #726

generate DetoxViewActions

7cd7704

Previously we had manually changed code in the generated sections.

DanielMSchmidt force-pushed the danielmschmidt/move-expect-to-generated-code branch from f072bf2 to 7cd7704 Compare December 11, 2018 12:42

rotemmiz merged commit 491bb52 into master Dec 11, 2018

rotemmiz added the domain: internal label Dec 11, 2018

rotemmiz changed the title ~~Move expect.js to generated code~~ Android: Move expect.js to generated code Dec 11, 2018

lock bot locked as resolved and limited conversation to collaborators Dec 14, 2018

noomorph deleted the danielmschmidt/move-expect-to-generated-code branch April 18, 2019 06:12

Android: Move expect.js to generated code #746

Android: Move expect.js to generated code #746

Conversation

DanielMSchmidt commented May 19, 2018

rotemmiz commented May 20, 2018

DanielMSchmidt commented May 20, 2018 • edited Loading

noomorph commented Jul 29, 2018

noomorph commented Jul 29, 2018 • edited Loading

DanielMSchmidt commented Jul 29, 2018

DanielMSchmidt commented Aug 8, 2018

DanielMSchmidt commented Aug 8, 2018

noomorph 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

DanielMSchmidt commented Aug 11, 2018

noomorph commented Aug 11, 2018

DanielMSchmidt commented Aug 11, 2018

DanielMSchmidt commented Sep 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noomorph left a comment

Choose a reason for hiding this comment

DanielMSchmidt commented Sep 7, 2018

noomorph commented Sep 8, 2018

DanielMSchmidt commented Sep 8, 2018

DanielMSchmidt commented Sep 8, 2018

DanielMSchmidt commented Nov 21, 2018

rotemmiz commented Nov 21, 2018

DanielMSchmidt commented Nov 21, 2018

DanielMSchmidt commented Nov 21, 2018

DanielMSchmidt commented Nov 25, 2018

rotemmiz 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

DanielMSchmidt commented May 20, 2018 •

edited

Loading

noomorph commented Jul 29, 2018 •

edited

Loading