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

[WIP] Integrate WDIO #1342

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

conradkirschner
Copy link
Contributor

@conradkirschner conradkirschner commented Jul 25, 2021

Reason behind that pullrequest is, to integrate wdio. This will allow BackstopJS to use selenium/webdriver standards.

  • define how config should look like
  • check if cloud provider will work
  • do just the minimal changes for now -> make it run with wdio and exclude features that are not possible (throw warning)
  • convert engine scripts from puppeteer to wdio
  • checking real app screenshots

Currently the branch is Work in Progress, but feel free to have a first look.

  • do screenshot with wdio and compare them
  • clearif if backstopjs maintainer will merge this idea or not, define requirements for merge

@garris I tried to do as less changed as possible, so you can faster review it.
One thing I saw is that comparing puppeteer and wdio might not so easy and require more changes, but I would still merge it and say "Use puppeteer with Firefox/Chrome or ANY browser/device with wdio)

Conrad Magnus Kirschner added 6 commits July 25, 2021 11:03
@conradkirschner
Copy link
Contributor Author

waiting for feedback.

current blocker is settting window size ....

@garris
Copy link
Owner

garris commented Jul 25, 2021

Wow. This looks like a lot of work. I think I'll add a /wdio branch. Will try get to this soon. Thank you.

@conradkirschner
Copy link
Contributor Author

not that much work (1 evening + 1 day) I just tried to replace puppeteer everwhere with wdio. so incremental exchange commands

puppeteer:
page.evaluate()

wdio:
browser.execute();

copied the engine scripts and converted them aswell ...

(I need to spend a bit more time on it, before I would remove the WIP - like the window size height thing is most confusing me)

But sure I will try to finish that pull request soon, but I like to make my work transparent (therefore WIP)

- added configurable wdio config
  (services will not work, they will need some kind of integration)
- height is still wrong in wdio
@conradkirschner
Copy link
Contributor Author

Some features are missing in wdio standalone, that should not be part of that project.
webdriverio/webdriverio#7185

@conradkirschner
Copy link
Contributor Author

As suggested in webdriverio/webdriverio#7185 this should not be part of wdio core, so I added service handling directly in backstopjs, need to verify if that solution works (just tried out with selenium-standalone-service)

Conrad Magnus Kirschner and others added 4 commits August 1, 2021 13:20
  process env variables
- moved wdio service shutdown to bettr position
- added sample configuration
- added todo for checking startup services
-
- cleaned up a bit
- [fixed] service handling of wdio-remote
Conrad Magnus Kirschner and others added 4 commits August 11, 2021 17:10
  As there are some possibilities I am not sure which one
  I would use, added warning for now that feature is not
  supported with webdriverio
- added test config for browserstack
Copy link
Owner

@garris garris left a comment

Choose a reason for hiding this comment

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

Left some comments -- thanks for working on this!

README.md Outdated
@@ -29,6 +29,9 @@
- Plays nice with CI and source control
- Run globally or locally as a standalone package app or `require('backstopjs')` right into your node app
- Incredibly easy to use: just 3 commands go a long long way!
- [BETA] Limited Support for WebDriver Protocol, based on wdio
- works as well with any wdio-(cloud)-service (see [wdio configuration]() )
- not compatible with puppeteer references pictures
Copy link
Owner

Choose a reason for hiding this comment

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

What does this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had some issues with directly comparing, backstopjs puppeteer pictures with wdio pictures.

  • screensize is different a bit (still need to investigate if that what goes on, but due sickness&work I couldn't find the time)
  • puppeteer does a full page picture, webdriver captures only viewport => In theory, should be possible to merge the viewport images to a full page picture, wdio plugin is outdated
  • image interception is not possible without a proxy solution, due selenium limitation

Therefore I added a BETA support, so maybe some people get interessted to work on it.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, it is very very unlikely that you could ever render a reference image in puppeteer and then be able to test that against a test image generated in WDIO. Or vice versa. In addition to different high level rendering interpretations of the different application environments, there will also be a low-level rendering issue because different hardware will dither bitmap text differently. For example you can see this explanation.

TLDR; Screenshots generated by a single hardware, software and platform combination can only be compared against itself.

core/command/approve.js Outdated Show resolved Hide resolved
core/util/makeConfig.js Show resolved Hide resolved
core/util/runWdio.js Show resolved Hide resolved
core/util/wdio-service-handler.js Outdated Show resolved Hide resolved
newdir/backstop_data/casper_scripts/onBefore.js Outdated Show resolved Hide resolved
@conradkirschner
Copy link
Contributor Author

@garris Regarding the different environments, I just added it into the readme to warn people that cross engine will not work.
#1342 (comment)

But as it is already written in the readme, might be better as a own section...
(removed it for now)

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.

2 participants