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

Bump top-level matchers api to upper Detox layer (where they should be) #2214

Merged
merged 1 commit into from
Jul 21, 2020

Conversation

d4vidi
Copy link
Collaborator

@d4vidi d4vidi commented Jul 15, 2020

  • This is a small change
  • This change has been discussed in issue #<?> and the solution has been agreed upon with maintainers.

Description

Relates to #2012: lays some preliminary ground work so that we could introduce it with proper API.

Dive-in

Looking at the current software architecture of Detox at a high-level perspective, we see two main distinct units, top-to-bottom:

  1. A framework for providing expect and element-interactions as a top-level, user-facing API (e.g. element(by.id(testId)).tap()), which in turn uses a distinct, platform-specific node-side (JS) implementation that eventually directly triggers device-side logic using invoke.
  2. A framework for providing element-less, device-scope, top-level, user-facing APIs (e.g. device.takeScreenshot()), which are provided mainly by the (currently) singleton Device object, that in turns delegates platform-specific logic to Drivers.

While the two global units are, in principle, agnostic, we've nevertheless created an artificial coupling between them, as the top-level implementation of unit 1 is created via the drivers of unit 2, due to convenience (example). Unfortunately, this type of an unnecessary coupling - as a rule of thumb, damages flexibility, and we can see it in the case of elements' screenshot.

The pseudo-code implementation of taking an element screenshot is as follows:
a. Invoke an on-test-device, native method that creates an on-device .png file.
b. Pull the image back to the host through the driver (e.g. using adb) and provide the path back to the caller.

Ideally, this top-level API for doing this would be amazing:

const screenshotFilePath = await element(by.id('fancy-element-id')).takeScreenshot()`

But due to the coupling mentioned earlier, this cannot be done, as the underlying Element.takeScreenshot() function that would implement this -- originally generated by driver, cannot access the device in part b of the pseudo-code (!) It more so artificially forces us onto this kind of an API, which is largely undesired as it is both not friendly and would encourage code duplication and single-responsibility breakage in Detox itself:

const screenshotFilePath = await device.takeScreenshot( element(by.id('fancy-element-id')) );

If we want to nevertheless stick to the API that we really want - and with that, empower high flexibility in Detox (yes, gimme all those things!), we need to break the coupling.

This is the scope of this PR: 100% Separating drivers and matchers, and their creation. Instead of having the matchers created by the drivers, we delegate the proper matcher resolution to a new util called matchersRegistry (equilvant to driverRegistry), which is utilized in the Detox class (aka our Detox main layer).

Unfortunately, the main drawback is a minor breakage of external-drivers implementation: Whichever plugin-driver implementations that exist out there in the wild -- they still provide the matchers through the driver. Fortunately, I've made the migration pretty easy. Given a plugin driver.js, users will have to apply these minor changes:

class Expect {
-  constructor(invocationManager) {
+  constructor({ invocationManager }) {
     this._invocationManager = invocationManager;
  }
}

class PluginDriver {
  constructor() {
-    this.matchers = new Expect(new invocationManager());
+    // no this.matchers init here...
  }
}

-module.exports = PluginDriver;
+module.exports = {
+  DriverClass: PluginDriver,
+  ExpectClass: Expect,
+}

@d4vidi d4vidi requested a review from noomorph July 15, 2020 18:05
@d4vidi d4vidi self-assigned this Jul 15, 2020
@d4vidi d4vidi force-pushed the element-screenshot branch 2 times, most recently from ad7a4ba to 0686834 Compare July 15, 2020 18:18
@rotemmiz
Copy link
Member

How does it relate to #2012?

@noomorph
Copy link
Collaborator

@rotemmiz, Amit wants to create a convenient DSL for taking element screenshots. He'll explain in depth if you want, but anyway I think that's the right direction.

@d4vidi d4vidi force-pushed the element-screenshot branch from 0686834 to 61e19cb Compare July 16, 2020 07:25
@d4vidi
Copy link
Collaborator Author

d4vidi commented Jul 16, 2020

@rotemmiz I'll write down an elaborate explanation asap!

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2456

  • 17 of 17 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 85.62%

Totals Coverage Status
Change from base Build 1298: 0.03%
Covered Lines: 3621
Relevant Lines: 4065

💛 - Coveralls

@d4vidi d4vidi marked this pull request as ready for review July 16, 2020 09:29
@d4vidi d4vidi requested a review from rotemmiz as a code owner July 16, 2020 09:29
@rotemmiz
Copy link
Member

I am still not convinced that this is an important change, though I am OK with whatever makes you happy :)

The API you mentioned await element(by.id('fancy-element-id')).takeScreenshot() is achievable without this change, as a new action (similar to the newly added element(matcher).getAttributes(). Or am I missing something?

@@ -111,6 +111,11 @@ describe('Device', () => {
expect(device.name).toEqual('mock-device-name-from-driver');
});

it('should return the type from the configuration', async () => {
const device = validDevice();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@d4vidi, just so I don't forget. How well does that .type implementation play with our recently added 3rd party device driver support? That is, where you can point Detox to JS module path with a custom driver implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. With this implementation it will return the relative-path as-is, which might be weird, but that wouldn't be the driver's fault (rather, the whole concept's "fault"). This is why I suggest to keep as-is. Nevertheless, it is overrideable by the driver's implementor.

this._artifactsManager = new ArtifactsManager(this._artifactsConfig);
this._artifactsManager.subscribeToDeviceEvents(this._eventEmitter);
this._artifactsManager.registerArtifactPlugins(deviceDriver.declareArtifactPlugins());

await this.device.prepare();

const matchers = matchersRegistry.resolve(this.device, {
invocationManager,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@d4vidi, so, your plan is to inject device or a similar handle there, to ensure that the matchers will be able to pull the files from the device, after the screenshot is done?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bingo! The restored flexibility would allow us to do so 💪

@d4vidi
Copy link
Collaborator Author

d4vidi commented Jul 19, 2020

@rotemmiz

The API you mentioned await element(by.id('fancy-element-id')).takeScreenshot() is achievable without this change

It can be defined and worked out to take the screenshot on the device, as you normally would, such that a a file will be created on the device (part A of my so-called pseudo-code). But it would not be able to pull the file back to the host, as it cannot access the device (part B of the peuso-code).

though I am OK with whatever makes you happy

  1. ❤️
  2. I put some more thought into it - it's not fair to say the two distinct units should be hard-separated; They don't have to be. Rather, the problem here is specifically with how in "unit 2"'s lowest layers (i.e. device driver), we currently create the top-level API of "unit 1" (i.e. the element/expect API's). That is a sheer violation of basic software architecture practices, and should be fixed even regardless of screenshots or any specific feature. The element screenshot simply surfaces that specific code-smell.
  3. The thing that would really make me happy is to separate all of our top-level API's onto an standalone API layer, and have everything handled by a lower-layer implementation, which would have the freedom to be platform-specific where needed. In other words, make the top-level API layer very thin, and extending the device/driver layers to serve all API's - not just device.xyz() methods.

@d4vidi d4vidi force-pushed the element-screenshot branch from 61e19cb to 5e69e58 Compare July 21, 2020 06:52
@d4vidi d4vidi merged commit f199c76 into master Jul 21, 2020
@d4vidi d4vidi deleted the element-screenshot branch July 21, 2020 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants