Add dynamic require for driver plugins by awinograd · Pull Request #1919 · wix/Detox

awinograd · 2020-02-20T16:19:31Z

This is a small change
This change has been discussed in issue Web support via Puppeteer #1882 and the solution has been agreed upon with maintainers.

Description:

This PR adds support for external drivers as discussed in #1882 (comment).

"ios.sim.release": {
        "binaryPath": "...",
        "build": "...",
        "type": "my-custom-driver-package-name",
        "device": {
	  // can be pretty flexible (according to the device driver), add whatever device specification needed
        }
      },

awinograd · 2020-02-20T16:22:09Z

detox/src/Detox.js

    await this._client.connect();

-    const DeviceDriverClass = DEVICE_CLASSES[this._deviceConfig.type];
+    const DeviceDriverClass = DEVICE_CLASSES[this._deviceConfig.type] || require(this._deviceConfig.type);


One thing I don't like about this direct fallback to require is you get a weird error about module resolution if you have a typo for one of the built-in drivers. In other words, the if on the line below is sort of useless...

@rotemmiz, what are your thoughts about requiring third party drivers to be prefixed in the configuration?

Something like:

"ios.sim.release": { "binaryPath": "...", "build": "...", "type": "plugin:my-custom-driver-package-name", "device": { // can be pretty flexible (according to the device driver), add whatever device specification needed } },

let DeviceDriverClass = DEVICE_CLASSES[this._deviceConfig.type]; if (!DeviceDriverClass) { if (this._deviceConfig.type.startsWith('plugin:')) { DeviceDriverClass = require(this._deviceConfig.type); } throw new Error(`'${this._deviceConfig.type}' is not supported`); }

awinograd · 2020-02-20T16:26:28Z

One other thing that would be great to resolve as part of this PR is the binaryPath issue discussed in #1882 (comment).

Essentially, I want a way to avoid this line which is currently mandatory but doesn't make sense in the context of a website which doesn't have a binaryPath. (Currently I'm using binary path to specify the base web url but have to add a leading / to make it absolute and avoid this logic. e.g. /https://example.com)

Detox/detox/src/devices/Device.js

Line 18 in 475d10e

this._binaryPath = this._getAbsolutePath(this._deviceConfig.binaryPath);

rotemmiz · 2020-03-01T09:06:51Z

Re binaryPath, let's make sure it is only used where needed (push it into the drivers maybe).
We can introduce appPath in Device instead, that will be used for binaryPath in iOS/Android drivers, and for url in your driver.
WDYT?

wix#1919 (comment)

awinograd · 2020-03-01T15:34:53Z

@rotemmiz thanks for the direction. I just pushed a commit that pushes absolute path logic into the driver layer (removing from device).

I wasn't sure exactly how you wanted me to handle binaryPath vs appPath

Did you want to totally remove binaryPath as an option from the config (a breaking change)?

Or add a new appPath field to config that takes precedence over and deprecates binaryPath for future removal.

In the grand scheme of things, it's currently not a huge issue to keep the binaryPath naming even though it perhaps isn't as accurate for my web driver, so I'm also happy to punt on that until the next major version is on its way.

Let me know how you'd like to proceed!

rotemmiz · 2020-03-08T12:36:22Z

Hey @awinograd, seems like a few unit tests failed, and coverage is missing as well.

…rtions also fix code coverage

awinograd · 2020-03-08T18:20:49Z

@rotemmiz, tests + coverage fixed

rotemmiz · 2020-03-11T10:32:22Z

Hey @awinograd, sorry for the delay here, I was trying to think and consult on how to proceed here.
So the things we think are still missing to make sure we handle this PR correctly are the following:

A new section in documentation, explaining how to create such a plugin, and most importantly, a section about how to use one.
A good integration test to make sure we don't break this on the next commit we make for Detox. So I suggest you add another project (either in examples dir on someplace else, but we just need to make sure it runs in CI as well), and add a very simple setup (a package.json, short e2e test, and an empty stub of a driver that just returns resolved promises on anything).

WDYT?

awinograd · 2020-03-11T16:03:39Z

@rotemmiz no need to apologize! I appreciate your guidance through the PR process.

Yes I can write up a doc on custom drivers
Do you want me to configure the setup on jenkins, or would a script in examples/custom-driver/package.json be sufficient for you to take it and get it on jenkins?

rotemmiz · 2020-03-11T16:36:41Z

Although it makes little sense to add the project to one of the platform specific test project CI builds, I believe it will be the fastest and easiest, and you'll be able to do it independently.

Try adding your new project to the list here, and see
https://github.com/wix/Detox/blob/master/scripts/demo-projects.android.sh

Need to maintain 100% coverage while not breaking prepare for drivers whose binaryPath isn't an actual path on disk

awinograd · 2020-03-23T22:07:28Z

@rotemmiz, sorry for the delay in getting back to this. I just pushed an example project (🤞 that the CI passes!)

Still need to work on the Doc, but wanted you to take a look at f014adc as I'm not happy with what i've done there. It's related to our discussion above (#1919 (comment)) about binaryPath. The old testing code assumes that prepare will throw if the binary path doesn't exist on disk which is an issue when binaryPath is a URL. I wasn't sure what the best path forward is, so just pushed something that passes but is sort of a hacky workaround. Open to suggestions there

awinograd · 2020-03-23T22:29:22Z

Just put up a first pass at a new Guide around Third party drivers. I'm curious to know what areas you think may be interesting to include that i've left off. As of now, it's a pretty high level summary.

awinograd · 2020-03-25T03:47:08Z

The CI is failing due to

19:05:05 /home/jenkins/.jenkins/workspace/detox-demo-projects-android-59-9-pr/examples/demo-puppeteer/node_modules/puppeteer/.local-chromium/linux-722234/chrome-linux/chrome: error while loading shared libraries: libXss.so.1: cannot open shared object file: No such file or directory

I see two potential solutions:

Install the missing package(s)
Change the demo to use a trivial, noop driver rather than detox-puppeteer

Let me know which direction you prefer. If it's (1), I'll need help

awinograd · 2020-03-30T01:17:17Z

I went ahead an chose path (2) since it seemed like simplest way to test the actual integration without worrying about the internal plugin details. If you'd like to go down path (1) and restore examples/demo-puppeteer let me know. It's just a simple revert away

awinograd · 2020-03-30T15:41:26Z

Most recent test failure seems unrelated to my changes

rotemmiz · 2020-04-01T16:31:48Z

Good call going with 2, we wouldn't want detox CI to be more complex than what it already is, this demo you added is there only to showcase the API and make sure we don't break it unintentionally

rotemmiz · 2020-04-01T16:37:24Z

Though the first error is probably not related (couldn't found the cause scrolling with my phone), the second (in demo projects build) looks related to your work

lerna.json

awinograd · 2020-04-09T15:39:32Z

I see a new coveralls ci process has been added to the PR and has failed. https://coveralls.io/builds/29961248

Should I be paying attention to it?

noomorph · 2020-04-10T10:43:19Z

@awinograd, thanks for telling. I'll look into it.

rotemmiz

Perfect documentation writing (Expect for a small terminology note), Kudos.
I added a few notes, and we're checking why CI fails, bare with us, we will merge this ASAP.

rotemmiz · 2020-04-12T20:45:57Z

detox/src/devices/Device.test.js

    const device = schemeDevice(configurationsMock.pathsTests, configuration);

-    await device.prepare();
+    await device.prepare({ checkBinaryPath: true });


Seems to me like checkBinaryPath doesn't really belong in here, is there any function call without checkBinaryPath:true ?
If this is to separate the web driver from iOS/Android drivers, then maybe this logic should reside in the drivers themselves.

I'm not ecstatic about how I had to get the tests passing. If I remember correctly, I was fighting against code coverage, but perhaps the real solution is to add a separate test for getAbsoluteBinaryPath

So does this mean you can get rid of checkBinaryPath: true?

sorry I missed this reference. yes we can get rid of it

rotemmiz · 2020-04-12T20:47:15Z

detox/src/devices/Device.test.js

      await device.installApp('newAppPath');

-      expect(driverMock.driver.installApp).toHaveBeenCalledWith(device._deviceId, 'newAppPath', undefined);
+      expect(driverMock.driver.installApp).toHaveBeenCalledWith(device._deviceId, 'newAppPath', device._deviceConfig.testBinaryPath);


Why did this change from undefined to testBinaryPath ?

Previously this expectation was relying on implementation details of the Device class. Specifically, it knew that testBinaryPath was not passed along to newAppPath if prepare has not yet been called. That's because previously, the binary path instance properties were set on the instance during prepare after doing the absolute path logic I have pushed down into the driver layer.

That seemed a bit odd to me, since in all real uses of Device (as far as I'm aware) prepare will always be called before using the other methods. This method expects the testBinaryPath as the last argument, it just previously happened to not be defined yet

rotemmiz · 2020-04-12T20:48:42Z

docs/Guide.ThirdPartyDrivers.md

@@ -0,0 +1,62 @@
+# Third Party Drivers
+
+Detox comes with built-in support for running on Android and iOS by choosing a driver type in your detox configurations. For example, the following configuration uses the "ios.simulator" driver.


"as of v16.(3?)"
We need to remember to add it

docs/Guide.ThirdPartyDrivers.md

…ce.test.js

awinograd · 2020-04-13T15:30:42Z

@rotemmiz thanks for the review! I've done a first pass at addressing your comments

rotemmiz · 2020-04-15T20:47:30Z

Can't yet understand why all builds failed, I will try figuring it out tomorrow morning.
Also, do we still need the checkBinaryPath flag in function calls?

awinograd · 2020-04-15T21:05:53Z

@rotemmiz just updated the PR to remove the remaining reference to checkBinaryPath. unit tests are passing locally for me.

thanks for taking a look at the remaining failures.

noomorph · 2020-04-17T16:35:53Z

@awinograd, the issue with Coveralls was finally solved - could you please merge master in?

awinograd · 2020-04-17T16:53:55Z

@noomorph . Sure thing. Just merged master

awinograd · 2020-04-17T22:46:53Z

Coveralls ran but failed here.

Jenkins CI is green again!

noomorph · 2020-04-18T12:22:16Z

@awinograd, thanks for reporting. I have found why the coverage was below 70ish% and pushed the fix. Could you please merge master again? Now it should pass the required coverage.

awinograd · 2020-04-18T14:42:20Z

merged!

d4vidi · 2020-04-19T12:33:27Z

examples/demo-plugin/e2e/init.js

+
+// This will post which device has assigned to run a suite, which can be useful in a multiple-worker tests run.
+// This is strictly optional.
+jasmine.getEnv().addReporter(assignReporter);


@noomorph another thing to keep up to date with respect to jasmine deprecation

awinograd · 2020-04-22T15:48:35Z

Anything else I can do here?

rotemmiz · 2020-04-22T16:52:52Z

🥳

awinograd · 2020-04-22T16:54:37Z

Amazing! Thanks for all of your help getting this change developed & merged! Excited to see it released officially 🎊

rotemmiz · 2020-04-22T16:59:05Z

Thank you for having such persistency in having this PR perfected.
We are happy this change is in.

d4vidi · 2020-04-23T08:58:39Z

W00t! Don't forget to bump a minor version for the next release :)

justinwyer · 2020-04-23T09:27:29Z

Amazing stuff @awinograd this is super stuff 😍thanks so much for driving this!

d4vidi · 2020-07-19T11:43:28Z

@awinograd @justinwyer please be advised that we're about to introduce a minor yet breaking change to the plugged-in drivers in #2214. The change it subtle and we will report the details in the release itself, but it will not be release as a major version.

Add dynamic require for driver plugins

b85fd7b

awinograd requested a review from rotemmiz as a code owner February 20, 2020 16:19

awinograd commented Feb 20, 2020

View reviewed changes

Push absolute path logic into driver layer

2833acb

wix#1919 (comment)

awinograd added 2 commits March 8, 2020 11:08

fix existing tests to use new internal implementation details in asse…

940e57e

…rtions also fix code coverage

Add test for support of driver plugin

528e545

awinograd added 2 commits March 23, 2020 16:04

Fix tests

f014adc

Need to maintain 100% coverage while not breaking prepare for drivers whose binaryPath isn't an actual path on disk

Add demo-puppeteer

47aa0c3

Create Guide.ThirdPartyDrivers.md

cb1266a

awinograd force-pushed the aw-driver-plugin branch from 8b3c27b to cb1266a Compare March 23, 2020 22:38

Use headless driver for jenkins compatibility

89bff34

awinograd added 2 commits March 29, 2020 19:15

Add demo-plugin project

b854400

Remove demo-puppeteer

b102660

Merge branch 'master' into aw-driver-plugin

e74ca19

bump detox to v16 in examples/demo-plugin

b3934d9

noomorph reviewed Apr 9, 2020

View reviewed changes

lerna.json Outdated Show resolved Hide resolved

remove demo-puppeteer from lerna.json

a16ff9b

rotemmiz suggested changes Apr 12, 2020

View reviewed changes

awinograd added 2 commits April 13, 2020 10:19

Update README with clarifications

3a5272c

Add getAbsoluteBinaryPath.test.js to avoid testing it's logic in Devi…

485058c

…ce.test.js

awinograd requested a review from rotemmiz April 15, 2020 14:29

remove checkBinaryPath argument

88ea1dc

awinograd force-pushed the aw-driver-plugin branch from 4c8c9ba to 88ea1dc Compare April 15, 2020 21:04

Merge branch 'master' into aw-driver-plugin

9aa3338

Merge branch 'master' into aw-driver-plugin

f704f99

d4vidi reviewed Apr 19, 2020

View reviewed changes

rotemmiz merged commit c878093 into wix:master Apr 22, 2020

lock bot locked as resolved and limited conversation to collaborators May 5, 2020

		@@ -0,0 +1,62 @@
		# Third Party Drivers

		Detox comes with built-in support for running on Android and iOS by choosing a driver type in your detox configurations. For example, the following configuration uses the "ios.simulator" driver.

Conversation

awinograd commented Feb 20, 2020

Uh oh!

awinograd Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awinograd commented Feb 20, 2020

Uh oh!

rotemmiz commented Mar 1, 2020

Uh oh!

awinograd commented Mar 1, 2020

Uh oh!

rotemmiz commented Mar 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awinograd commented Mar 8, 2020

Uh oh!

rotemmiz commented Mar 11, 2020

Uh oh!

awinograd commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rotemmiz commented Mar 11, 2020

Uh oh!

awinograd commented Mar 23, 2020

Uh oh!

awinograd commented Mar 23, 2020

Uh oh!

awinograd commented Mar 25, 2020

Uh oh!

awinograd commented Mar 30, 2020

Uh oh!

awinograd commented Mar 30, 2020

Uh oh!

rotemmiz commented Apr 1, 2020

Uh oh!

rotemmiz commented Apr 1, 2020

Uh oh!

Uh oh!

awinograd commented Apr 9, 2020

Uh oh!

noomorph commented Apr 10, 2020

Uh oh!

rotemmiz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

awinograd commented Apr 13, 2020

Uh oh!

rotemmiz commented Apr 15, 2020

Uh oh!

awinograd commented Apr 15, 2020

Uh oh!

noomorph commented Apr 17, 2020

Uh oh!

awinograd commented Apr 17, 2020

Uh oh!

awinograd commented Apr 17, 2020

Uh oh!

noomorph commented Apr 18, 2020

Uh oh!

awinograd commented Apr 18, 2020

Uh oh!

awinograd Feb 20, 2020 •

edited

Loading

rotemmiz commented Mar 8, 2020 •

edited

Loading

awinograd commented Mar 11, 2020 •

edited

Loading