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

Store element-screenshot results as artifacts #2226

Merged
merged 2 commits into from
Jul 26, 2020

Conversation

d4vidi
Copy link
Collaborator

@d4vidi d4vidi commented Jul 23, 2020


Description:
Resolves #2012. Applies the final piece of this puzzle: the administration of artifacts management we have over image-files generated by using this feature.

@d4vidi d4vidi requested a review from noomorph July 23, 2020 10:12
@d4vidi d4vidi requested a review from rotemmiz as a code owner July 23, 2020 10:12
@d4vidi d4vidi self-assigned this Jul 23, 2020
@@ -167,6 +167,7 @@ class Detox {

const matchers = matchersRegistry.resolve(this.device, {
invocationManager,
emitter: this._eventEmitter,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

such flexibility, much design 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

exactly! :doge:

@d4vidi d4vidi force-pushed the element-screenshot-artifact branch from 3c89942 to bed5eda Compare July 23, 2020 10:15
@d4vidi
Copy link
Collaborator Author

d4vidi commented Jul 23, 2020

@noomorph with device.takeScreenshot() - do we e2e-test that the artifact is indeed generated, and with the proper name? If not, should we? It's tricky to do so, but it can be done I believe.

@noomorph
Copy link
Collaborator

noomorph commented Jul 23, 2020

@d4vidi , we have E2Es for the API itself (look for e2e/2*****screenshots.test.js, don't remember exactly), but since 2019 probably we abandoned snapshot tests for artifacts folder contents. I am looking much much very very forward to rewriting artifacts subsystem and making it much more generic in terms of creating, pulling, and saving the per-test-session and per-single-test artifacts, though after the rewrite we might no longer have the same artifacts outputs, though. 🤔

Why? Because there's a long ongoing story about making all glued for logs, video and Detox Instruments profiler recordings with some rich markers to navigate between nested suites, tests (and hooks), time markers and actors (Node.js processes, devices, apps, etc), need to put some thinking in how it is going to look like at the end of the day.

@@ -242,12 +242,16 @@ jestExpect(multipleMatchedElements.elements.length).toBe(5);
jestExpect(multipleMatchedElements.elements[0].identifier).toBe('FirstElement');
```

### `takeScreenshot()` Android Only
### `takeScreenshot(name)` Android Only
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest using takeScreenshot([name]) square brackets convention for marking optional args.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

np

Take a screenshot of the native view associated with the element in question. Useful for highly-focused visual comparison tests (i.e. comparison between elements rather than of complete screen, with visual "noise").
Takes a screenshot of the native view associated with the element in question, and schedules putting it to the [artifacts folder](https://github.com/wix/Detox/blob/master/docs/APIRef.Artifacts.md#enabling-artifacts) upon completion of the current test. Useful for highly-focused visual comparison tests (i.e. comparison between elements rather than of [complete screens](https://github.com/wix/Detox/blob/master/docs/APIRef.DeviceObjectAPI.md#devicetakescreenshotname) with "visual noise").

`name (optional)` - Name of the final file to store as the image artifact. For example, `my-text-field` would result in a file named `my-text-field.png`. In case the name isn't provided, Detox would self-generate a distinct name, instead (though not a very descriptive one).
Copy link
Collaborator

Choose a reason for hiding this comment

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

would self-generate a distinct name, instead (though not a very descriptive one).

generates a random alphanumeric name instead.

@noomorph
Copy link
Collaborator

@d4vidi, I am sorry in advance for piggybacking, but could you also make device.takeScreenshot(name) optional in a sense that it will be generating a random name for the screenshot, to align the APIs? What do you think?

@noomorph
Copy link
Collaborator

noomorph commented Jul 23, 2020

@d4vidi, side thought — do we ignore/noop element().takeScreenshot()for iOS?

@noomorph
Copy link
Collaborator

Looks good, overall.

@noomorph
Copy link
Collaborator

Regarding:

Resolves #2012

Just musing, whether we consider #2012 to resolved without having iOS feature parity... ? 🤔

@d4vidi
Copy link
Collaborator Author

d4vidi commented Jul 23, 2020

@noomorph 3d6063f
✅ Piggybacking
✅ iOS no-op (actually, decided to throw an error because takeScreenshot() should return a value)
✅ Device-API doc
❌ Resolving #2012 - let's open a separate issue for that, as we've done in the past with Android

Copy link
Collaborator

@noomorph noomorph left a comment

Choose a reason for hiding this comment

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

I dig it.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2485

  • 9 of 9 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 85.644%

Totals Coverage Status
Change from base Build 1304: -0.007%
Covered Lines: 3636
Relevant Lines: 4080

💛 - Coveralls

@d4vidi d4vidi merged commit ea6a97e into master Jul 26, 2020
@d4vidi d4vidi deleted the element-screenshot-artifact branch July 26, 2020 07:00
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.

[feature] Element Screenshot
3 participants