Element screenshot (Android) #2222

d4vidi · 2020-07-21T12:24:10Z

This is a small change
This change has been discussed in issue [feature] Element Screenshot #2012 and the solution has been agreed upon with maintainers.

Description:
Relates to #2012: introduces the fully-blown (almost*) element screenshot taking solution for Android, using an interaction API.

Implementation notes:
After some research, files on Android are a bit of a headache, and really - there's no need for a file on the device. I therefore took @valentynberehovyi's advice and skipped file saving altogether, and instead return the bitmap as the invocation's result (base-64 encoded, of course). I tested this even with an element that takes up almost the entire screen on a full HD device (1920x1080), and it went smooth.

P.S.
PR 2222, best ever

* see caveats in documentation

detox/android/detox/src/main/java/com/wix/detox/common/ViewScreenshot.kt

d4vidi · 2020-07-21T12:38:35Z

detox/android/detox/src/main/java/com/wix/detox/espresso/EspressoDetox.java


-    public static ViewInteraction perform(ViewInteraction interaction, ViewAction action) {
-        return interaction.perform(action);


This never made sense! interaction.peform() always returns the interaction itself. What was the point here, to begin with? Effectively, invoke cannot parse an object, and therefore the final returned invoke-result has always been null.

With this new implementation, it's possible to return a result from the action itself. In the future, this should be useful for implementing getAttributes() @rotemmiz

d4vidi · 2020-07-21T12:41:42Z

detox/src/android/expect.js

 class Interaction {
  constructor(invocationManager) {
    this._call = undefined;
    this._invocationManager = invocationManager;
  }

  async execute() {
-    await this._invocationManager.execute(this._call);
+    const resultObj = await this._invocationManager.execute(this._call);
+    return resultObj ? resultObj.result : undefined;


This enables effectively using the result returned from invoke action on the device

d4vidi · 2020-07-21T12:42:51Z

detox/src/android/expect.test.js

-    await e.element(e.by.id('ScrollView799')).swipe('left', 'fast', 0.9);
-    await e.element(e.by.id('ScrollView799')).swipe('right', 'slow', 0.9);
-    await e.element(e.by.id('ScrollView799')).atIndex(1);
+  describe('element interactions', () => {


more boyscouting here than really related to this feature, except that I wanted more specific tests related to this specific interaction (screenshot taking)

d4vidi · 2020-07-21T13:11:01Z

@ethanshar

rotemmiz

Looks very good IMO, the only thing that bothers me is the API . element(matcher).takeScreenshot() is confusing with device.taakeScreenshot().
Also, this is a view bitmap, not a screen-shot, so I suggest chaging the name to element(matcher).takeViewBitamp(), element(matcher).getViewBitamp(), element(matcher).captureView() or something similar.

d4vidi · 2020-07-21T14:21:59Z

@rotemmiz how about element(matcher).captureBitmap()?

rotemmiz · 2020-07-21T14:22:35Z

Also great

d4vidi · 2020-07-21T14:23:16Z

Duly noted!

d4vidi · 2020-07-21T14:53:59Z

@rotemmiz done!

LeoNatan · 2020-07-21T18:16:11Z

@rotemmiz As opposed to screenshot, which is not a bitmap?

“Bitmap” is a bad terminology. It should be “snapshot”.

rotemmiz · 2020-07-21T19:19:28Z

That's why I suggested that view will be a part of the function name. We want to get a bitmap of the view. I think it should be with little ambiguity as possible.
Why do you think snapshot is a better terminology?

LeoNatan · 2020-07-21T19:39:12Z

I think we should be using the artifact system for this, and so “bitmap” is a very specific term. Images won’t be bmp, probably.

LeoNatan · 2020-07-21T19:40:13Z

View is also something we avoid everywhere, so we shouldn’t use that. snapshot() is enough, and perhaps provide file name as a parameter. Just like we don't use tapView() terminology.

noomorph · 2020-07-22T06:10:55Z

detox/src/utils/easy-file-io.js

+function saveRawBase64Data(dataBase64, { filePath, fileSuffix }) {
+  const _filePath = filePath || tempfile(fileSuffix);
+  const data = Buffer.from(dataBase64, 'base64');
+  fs.writeFileSync(_filePath, data);


How do you clean up temporary files after the test session ends?

Any good sense to bother with Buffer conversions?

fs.writeFileSync('hello.txt', 'SGVsbG8sIHdvcmxkIQ==', 'base64') fs.readFileSync('hello.txt', 'utf8'); // 'Hello, world!'

😻 😻 😻

d4vidi · 2020-07-22T07:28:03Z

@rotemmiz I tend to agree with @LeoNatan:

View = implementation details. Our API's offer an abstraction.
Bitmap is good, but might be a bit too technical. I favoured it because it hints of that we do not effectively grab pixels off the screen - but rather redraw the element onto a bitmap. We should probably come up with something between bitmap and screenshot. I'm open to suggestions.

As per snapshot - I'm highly against that because the term is used in snapshot testing. Moreover, this feature in particular, aims at providing a means for app-devs to take screenshots in order to perform snapshot tests. We should really avoid the term here.

LeoNatan · 2020-07-22T07:30:04Z

So the feature is intended for snapshot testing, but snapshot is not a good term? I don't understand.

LeoNatan · 2020-07-22T07:34:53Z

Apple uses "screenshot" terminology:

https://developer.apple.com/documentation/xctest/xcuiscreenshotproviding

https://developer.apple.com/documentation/xctest/xcuiscreenshot

noomorph · 2020-07-22T09:16:53Z

detox/src/utils/easy-file-io.test.js

+      uut = require('./easy-file-io');
+    });
+
+    it('should save raw data to a given file', () => {


Where do you use this scenario, Amit? @d4vidi

Right. I'll remove support for that.

noomorph · 2020-07-22T09:33:52Z

detox/src/android/expect.js

+  async captureBitmap() {
+    // TODO this should be moved to a lower-layer handler of this use-case
+    const resultBase64 = await new ActionInteraction(this._invocationManager, this, new CaptureBitmapAction()).execute();
+    return ezFile.saveRawBase64Data(resultBase64, { fileSuffix: '.detox.element.png' });


Could not we just move detox/src/artifacts/utils/temporaryPath.js to detox/src/utils, import it:

const temporaryPath = require('../utils/temporaryPath');

and just use in captureBitmap:

const resultBase64 = await new ActionInteraction(this._invocationManager, this, new CaptureBitmapAction()).execute(); const pngPath = temporaryPath.for.png(); await fs.writeFile(pngPath, resultBase64, 'base64'); return pngPath;

and not create yet another mini-module, please?

I considered doing that a number of times, but temporaryPath seemed very artifact oriented, seeing that the API set basically pairs 1:1 with the artifacts types we have (image, log, video, perf recording). If you find it apt nevertheless, I will use it here.

The mini-module was a compromise as we do not have a lower-layer logic-provider for this, just yet. I wanted to keep the logic here down to the bare minimum. 'Makes it easier to test (when in a separate module), too. wdyt?

I just don't see what it is here to test. 🤷‍♂️

The test idea here would be to check whether we write a received (mocked) base64 string to some temporary file.

If we extract logic to a separate file, it feels like we are going to write very similar tests (hence, 2 times the same) - for expect.js's takeScreenshot and that minimodule.

I don't feel like expanding the test logic by extra 3-4 lines is going to hurt its legibility:

ezFileIo = require('../utils/easy-file-io'); ezFileIo.saveRawBase64Data.mockReturnValue('/mock/file/path'); // ...

jest.mock('fs-extra').mock('../utils/temporaryPath'); require('../utils/temporaryPath').for.png.mockReturnValue('mock.png'); // ... expect(require('fs-extra').writeFile).toHaveBeenCalledWith('mock.png', invokeResultInBase64, 'base64');

noomorph

@d4vidi, if you are hesitant to change that ez pz unit stuff, I am not happy about it, but whatever you decide - it's up to you.

You have my approval.

d4vidi · 2020-07-22T13:41:20Z

@noomorph I'll take your advice on this and get rid of easy-file-io :)

noomorph · 2020-07-22T13:52:09Z

@d4vidi, 🙏 I sincerely hope I have managed to convey why I considered easy-file-io impractical a bit in a sense of inflating code and unit tests base with minimal benefits in return. Good luck with the field testing of the feature! Hope it works out as our guys expect.

detox/src/android/expect.test.js

d4vidi · 2020-07-22T14:47:33Z

@noomorph I generally agree. Again, in this case, this came up trying to work-around the fact we don't have proper abstractions in the expectation/matching realms, namely - with a proper place to put this logic in (I still don't think the Element.takeScreenshot() is the right place, at least no as-is).

But after pointing this out to me, I've come to terms that this can only be worked-out using proper refactoring.

…point

This reverts commit e56da45.

d4vidi requested review from rotemmiz and noomorph July 21, 2020 12:24

d4vidi self-assigned this Jul 21, 2020

d4vidi added type: enhancement ⚡️ platform: android labels Jul 21, 2020

d4vidi commented Jul 21, 2020

View reviewed changes

detox/android/detox/src/main/java/com/wix/detox/common/ViewScreenshot.kt Outdated Show resolved Hide resolved

d4vidi commented Jul 21, 2020

View reviewed changes

d4vidi changed the title ~~Element screenshot2~~ Element screenshot (Android) Jul 21, 2020

rotemmiz requested changes Jul 21, 2020

View reviewed changes

rotemmiz approved these changes Jul 21, 2020

View reviewed changes

noomorph reviewed Jul 22, 2020

View reviewed changes

noomorph approved these changes Jul 22, 2020

View reviewed changes

d4vidi force-pushed the element-screenshot2 branch 2 times, most recently from 8508450 to 6f60834 Compare July 22, 2020 13:40

noomorph reviewed Jul 22, 2020

View reviewed changes

detox/src/android/expect.test.js Outdated Show resolved Hide resolved

d4vidi force-pushed the element-screenshot2 branch from ad10941 to 10a6171 Compare July 22, 2020 14:33

d4vidi added 9 commits July 22, 2020 17:54

Introduce view-taking action on native side

def4f76

Introduce new interaction API over native-side element screenshot end…

9cd4647

…point

Add top-level API documentation

219f043

Fix broken unit test and do some renames

ab1cc17

Rename to .captureBitmap()

a313b46

Rename back to .takeScreenshot()

6d2cd83

This reverts commit e56da45.

Fix previously ignored view-clipping

48b502d

Kill easy-file-io JS-util module

8177a18

Improve feature documentation

f3a6ceb

d4vidi force-pushed the element-screenshot2 branch from 022da93 to f3a6ceb Compare July 22, 2020 14:54

d4vidi merged commit a4383ed into master Jul 23, 2020

d4vidi deleted the element-screenshot2 branch July 23, 2020 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Element screenshot (Android) #2222

Element screenshot (Android) #2222

d4vidi commented Jul 21, 2020 •

edited

Loading

d4vidi Jul 21, 2020 •

edited

Loading

d4vidi Jul 21, 2020

d4vidi Jul 21, 2020

d4vidi commented Jul 21, 2020

rotemmiz left a comment

d4vidi commented Jul 21, 2020

rotemmiz commented Jul 21, 2020

d4vidi commented Jul 21, 2020

d4vidi commented Jul 21, 2020

LeoNatan commented Jul 21, 2020

rotemmiz commented Jul 21, 2020

LeoNatan commented Jul 21, 2020

LeoNatan commented Jul 21, 2020 •

edited

Loading

noomorph Jul 22, 2020

noomorph Jul 22, 2020

d4vidi Jul 22, 2020

d4vidi commented Jul 22, 2020

LeoNatan commented Jul 22, 2020

LeoNatan commented Jul 22, 2020 •

edited

Loading

noomorph Jul 22, 2020

d4vidi Jul 22, 2020

noomorph Jul 22, 2020

d4vidi Jul 22, 2020

noomorph Jul 22, 2020

noomorph left a comment

d4vidi commented Jul 22, 2020

noomorph commented Jul 22, 2020

d4vidi commented Jul 22, 2020


		public static ViewInteraction perform(ViewInteraction interaction, ViewAction action) {
		return interaction.perform(action);

Element screenshot (Android) #2222

Element screenshot (Android) #2222

Conversation

d4vidi commented Jul 21, 2020 • edited Loading

d4vidi Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d4vidi commented Jul 21, 2020

rotemmiz left a comment

Choose a reason for hiding this comment

d4vidi commented Jul 21, 2020

rotemmiz commented Jul 21, 2020

d4vidi commented Jul 21, 2020

d4vidi commented Jul 21, 2020

LeoNatan commented Jul 21, 2020

rotemmiz commented Jul 21, 2020

LeoNatan commented Jul 21, 2020

LeoNatan commented Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d4vidi commented Jul 22, 2020

LeoNatan commented Jul 22, 2020

LeoNatan commented Jul 22, 2020 • 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

noomorph left a comment

Choose a reason for hiding this comment

d4vidi commented Jul 22, 2020

noomorph commented Jul 22, 2020

d4vidi commented Jul 22, 2020

d4vidi commented Jul 21, 2020 •

edited

Loading

d4vidi Jul 21, 2020 •

edited

Loading

LeoNatan commented Jul 21, 2020 •

edited

Loading

LeoNatan commented Jul 22, 2020 •

edited

Loading