Skip to content

test(docs): add visual testing using applitools #4872

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

Merged
merged 3 commits into from
Sep 5, 2019

Conversation

Nkemjiks
Copy link
Contributor

Description:
Hi everyone, this PR adds visual regression testing using (Applitool)[https://applitools.com/] eyes. If you are unfamiliar with Visual Regression testing, here a list of references from Applitools to learn more about it.

https://applitools.com/docs/topics/overview/overview-visual-testing.html
https://applitools.com/blog/troubleshoot-fix-react-bugs
https://circleci.com/gh/myspivey/lock/6

As an open source tool, this service will be offered to you free with an OSS license. They do offer a generous free tier if you want to get a license now to play around with it. I used protractor as the browser test harness because Applitools has an SDK for it.

The process for this testing is the docs site is served using concurrently and the existing design script, then protractors launches a headless Chrome where it runs the tests. Applitool's SDK then takes DOM snapshots w/ styles (not screenshots) from that headless Chrome and sends them to Applitool's servers where it renders those results in Chrome and Firefox and then takes a screenshot of that result and does its intelligent comparisons against the baseline. The baseline is created the first time you run it. If there's an issue, it'll be reported in CircleCI with an included link directly to Applitool's web app where you can see the visual differences and either accept or reject them or do other cool things like set ignored regions or tweak the matching behavior.

You'll need to create an Applitools account and export APPLITOOLS_API_KEY=Your_API_Key_Here in your local environment if you run it locally.

To run the test locally, go to your directory and run:
npm run e2e

Related issue (if exists):

@ladyleet
Copy link
Member

@benlesh do you have an idea of when this will get merged?

Copy link
Member

@niklas-wortmann niklas-wortmann left a comment

Choose a reason for hiding this comment

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

Hi @Nkemjiks, thanks a lot for this pr! I really appreciate the work you put into that! It's super valuable for the docs.

At some point, I would like to this as a quality gate in the ci pipeline, but this can be done in a separate pr.

The code itself looks quite good. It just seems like there is a missing dependency, Despite that I'm just not able to execute it locally. I'm trying to run in on a windows 10 machine and run into the following error when executing npm run e2e:

events.js:174
      throw er; // Unhandled 'error' event
      ^

Error: write EPIPE
    at ChildProcess.target._send (internal/child_process.js:750:20)
    at ChildProcess.target.send (internal/child_process.js:634:19)
    at Observable.rxjs_1.Observable.obs [as _subscribe] (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\src\utils\run-module-as-observable-fork.js:57:23)
    at Observable._trySubscribe (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\Observable.js:43:25)
    at Observable.subscribe (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\Observable.js:29:22)
    at C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\util\subscribeTo.js:22:31
    at Object.subscribeToResult (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\util\subscribeToResult.js:7:45)
    at MergeMapSubscriber._innerSub (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\operators\mergeMap.js:75:38)
    at MergeMapSubscriber._tryNext (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\operators\mergeMap.js:72:14)
    at MergeMapSubscriber._next (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\operators\mergeMap.js:55:18)
    at MergeMapSubscriber.Subscriber.next (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\Subscriber.js:64:18)
    at MergeMapSubscriber.notifyNext (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\operators\mergeMap.js:84:26)
    at InnerSubscriber._next (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\InnerSubscriber.js:25:21)
    at InnerSubscriber.Subscriber.next (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\Subscriber.js:64:18)
    at C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\util\subscribeTo.js:17:28
    at Object.subscribeToResult (C:\dev\rxjs\docs_app\node_modules\@angular-devkit\build-angular\node_modules\rxjs\internal\util\subscribeToResult.js:7:45)
Emitted 'error' event at:
    at process.nextTick (internal/child_process.js:754:39)
    at process._tickCallback (internal/process/next_tick.js:61:11)

Might be that this is an issue with my local machine, do you have any idea what might be causing this issue?

@@ -0,0 +1,41 @@
import { browser } from 'protractor';

const Eyes = require('eyes.selenium').Eyes;
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this isn't added to the dependency list in the package.json.

@Nkemjiks Nkemjiks force-pushed the add-visual-testing branch from 52e2b5f to e02bb92 Compare June 25, 2019 08:52
@Nkemjiks
Copy link
Contributor Author

Hi @jwo719 , thanks for pointing out the missing dependency. I was having issues running the already included e2e test and was doing alot of reverting back. I had installed the dependency locally before reverting but i didn't delete my node_modules that is why i didn't get the errors. I have updated the PR to include the dependency. Please pull and run again. Do let me know if you have any more issues

@ladyleet
Copy link
Member

ladyleet commented Jul 5, 2019

@jwo719 new status at all? // @Nkemjiks @myspivey

@niklas-wortmann
Copy link
Member

Hi @Nkemjiks, still I'm not able to run the tests locally. I'm still running in the same error, described above. I know remote debugging is kinda hard. If you like to we can do a screensharing session and investigate together what's causing the error?

@Nkemjiks
Copy link
Contributor Author

Nkemjiks commented Jul 8, 2019

Hi @Nkemjiks, still I'm not able to run the tests locally. I'm still running in the same error, described above. I know remote debugging is kinda hard. If you like to we can do a screensharing session and investigate together what's causing the error?

What time will you be available tomorrow so we can look into this?

@niklas-wortmann
Copy link
Member

Sorry tomorrow might be a little bit difficult. It's propably easier to make an appointment more in private. You can contact me on twitter @niklas_wortmann or in the reactivex slack channel (I think @ladyleet could invite you)

@niklas-wortmann
Copy link
Member

Together with @Nkemjiks we found out that my local issues were caused by an angular/cli bug
Unfortunately to fix this, we need to merge #4688 before. I will focus on merging #4688 so that I can test this one again. Sorry for keeping this on hold for the meantime.

@benlesh
Copy link
Member

benlesh commented Jul 31, 2019

Ping @jwo719

@ladyleet
Copy link
Member

Hey all! Just pinging on this.

@niklas-wortmann
Copy link
Member

So the blocking pr is finally merged (sorry for the delay): I gave it a quick try and noticed some things:

  • this pr needs a rebase
  • due to some compilation errors the other e2e spec files needs to be deleted

@Nkemjiks Could you take care of it? Many thanks for your contribution!

@Nkemjiks Nkemjiks force-pushed the add-visual-testing branch from e02bb92 to 404212c Compare August 30, 2019 10:32
@Nkemjiks
Copy link
Contributor Author

Nkemjiks commented Aug 30, 2019

@jwo719 , I have rebased and deleted the other spec file as requested. I don't know why the typescript check is failing though..

@cartant
Copy link
Collaborator

cartant commented Aug 31, 2019

@Nkemjiks I wouldn't worry about the TypeScript error. The release of TypeScript 3.6 has effected some errors with RxJS due to what appears to be a TypeScript bug - see #4992. (It looks like the CI build script runs npm i - which, IIRC, upgrades to the latest versions that match the semver range, so the CI build is most likely using TS 3.6.)

@jwo719 Let's ignore the TypeScript error and merge this if everything else is good to go.

@niklas-wortmann niklas-wortmann merged commit fc3d426 into ReactiveX:master Sep 5, 2019
@niklas-wortmann
Copy link
Member

Thank you very much @Nkemjiks for all the work you put into that and I'm really sorry for all the hassle!

@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants