Initial commit adding integration tests. #1557

jrbenny35 · 2020-02-18T22:44:19Z

Adds a total of 6 tests. 3 are visual regression tests that use a base image screenshot, The others are using selenium to validate the page loads the correct data

should load correct number of breaches from an email input: Checks the number of breaches shown matches the number.
should allow secondary email to be added: Flow that logs in and adds an email from the user dashboard.
should load the latest breach card: Checks breach card loads on home page

pdehaan

Q: Do we need "tests/integration/results/allure-results/wdio-0-0-allure-reporter.log"?

LOL at all the floating headers in the screenshots.

I'm not sure the Breaches_Page baselines will really work in the long run. It seems like any recent breach will cause the items to reorder and we'll need to regen the baselines after each freshly added breach. Possibly multiple times after we upload a new breach logo to the CDN.

Even the Home_Page baseline has a recent breach card. Not sure how viable it would be to hide that DOM element before taking/comparing images, or if that would defeat the purpose and reduce confidence (less brittle test that doesn't test a core thing). 🤷‍♂

package.json

pdehaan · 2020-02-18T23:04:25Z

tests/integration/pages/desktop/dashboard.page.js

@@ -0,0 +1,22 @@
+/* global $ */


Was this just to appease the ESLint gods, or was this needed for something else?

For eslint. The $ is needed for wdio though

Ah, OK. You can also define [per-glob] globals in the ESLint config file (.eslintrc.js in root of project) instead of needing to define in each template.

For example, in the .eslintrc.js overrides: section, you can tweak the tests/**/*.js glob (around L32) and inject your $, $$, and browser globals there — if that is easier.

{ files: [ 'tests/**/*.js', ], env: { jest: true, }, + globals: { + "$": "readonly", + "$$": "readonly", + "browser": "readonly", + }, },

Well that's pretty neat!

pdehaan · 2020-02-18T23:10:27Z

tests/integration/pages/desktop/home.page.js

+  get breachCard() { return new BreachCard(); }
+  get breachEmailAddress() { return $("#scan-email"); }
+  get checkBreachesButton() {
+    $(".input-group-button > input:nth-child(1)").click();


n-th child!
Not sure if we want to put a more convenient class/handle on that submit button to make this any easier on you.
Or not sure if we can abuse the system with something like:

$(".input-group-button > input[type='submit']).click(); // if this even works

I'd be reluctant to bend it too far and try reading the button's label, since that will change with different locales (ie: input[value='Check for Breaches'] feels a bit risky risky).

Classes on everything would make it easier but I know that is boring so..this works, and if it gets updated it's an easy change

pdehaan · 2020-02-18T23:19:50Z

tests/integration/pages/desktop/scanResults.page.js

+  }
+
+  get numberOfBreaches() {
+      return $(".headline > span:nth-child(1)").getText();


tests/integration/tests/test-create-baseline.js

jrbenny35 · 2020-02-19T17:06:37Z

tests/integration/tests/test-home-page.js

+      $(".email").click();
+      $(".email").setValue(global.primaryEmail);
+      $("#submit-btn").click();
+    } catch (error) {


Sometimes the email doesn't show as monitor passes the email you used for checking breaches into the fxa sign in

pdehaan

This looks great, @jrbenny35!
Thanks for all your hard work on this!

tests/integration/regions/navbar.region.js

tests/integration/wdio.conf.js

tests/integration/tests/test-breaches-page.js

tests/integration/tests/test-create-baseline.js

pdehaan · 2020-02-19T00:13:57Z

tests/integration/tests/test-home-page.js

+const UserDashboardPage = require("../pages/desktop/dashboard.page");
+
+describe("Firefox Monitor homepage", function() {
+  //this.retries(2);


Should this be uncommented (like in L7 of previous file)?

I am thinking retries aren't so bad, as there are multiple things that could go wrong. What do you think @pdehaan

👍 on retries, and 2 is a good number.

.circleci/config.yml

groovecoder · 2020-02-19T15:01:29Z

scripts/wait-for-it.sh

@@ -0,0 +1,182 @@
+#!/usr/bin/env bash


I'm always nervous about adding shell scripts to repos. Can we add this WITHOUT executable permission, and then have the docker file change it to executable?

OK, after marinating on this overnight and giving it a few Bing searches, what about something like this: https://www.npmjs.com/package/wait-on

wait-on is a cross-platform command line utility which will wait for files, ports, sockets, and http(s) resources to become available (or not available using reverse mode). Functionality is also available via a Node.js API. Cross-platform - runs everywhere Node.js runs (linux, unix, mac OS X, windows)

I've never used it in a project yet, but 🤷‍♂.

Our current implementation looks something like this:

command: ["./scripts/wait-for-it.sh", "postgres:5432", "--", "npm", "run", "db:migrate"]

Not sure if that would be similar to:

command: ["wait-on", "tcp:5432", "&&", "npm", "run", "db:migrate"]

Or if we just add another npm script in package.json which does "ci:db:migrate" so the command looks less scary.

Not sure what would be best here. I am open to suggestions.

@jrbenny35 and I were noodling on this a bit, and we might be able to remove the +x flag on the .sh file and just invoke this script using bash ./wait-for-it.sh. Testing locally, that seems to still work in a simple test case even though the file is non-executable.

tests/integration/wdio.conf.js

+
+      global.baseUrl = "http://localhost:6060";
+      global.primaryEmail = "test@mailinator.com";
+      global.secondaryEmail = "test" + Math.random() + "@mailinator.com";


tests/integration/wdio.conf.js

+
+      global.baseUrl = "http://localhost:6060";
+      global.primaryEmail = "test@mailinator.com";
+      global.secondaryEmail = "test" + Math.random() + "@mailinator.com";


tests/integration/tests/test-home-page.js

groovecoder · 2020-02-19T15:11:34Z

tests/integration/wdio.conf.js

+      global.primaryEmail = "test@mailinator.com";
+      global.secondaryEmail = "test" + Math.random() + "@mailinator.com";
+      global.mailinatorPassword = process.env.MAILINATOR_PASSWORD || "a_secure_password";
+      browser.setWindowSize(1920, 1080);


for a future enhancement, we might want to run tests with mobile-dimension window sizes. or do we use another mechanism for testing mobile browsers/dimensions?

For mobile I'll have to set a screensize as well. We could maybe have another *.conf.js for mobile that has that set differently.

@groovecoder did you have a specific device/viewport in mind?
Somewhere between an Alcatel Go Flip 2 and a Samsung Galaxy S20 Ultra 5G Turbo: Hyper Fighting edition?

I usually use an iPhone 'Plus' model

Looks like the latest iPhone [11 Pro Max], has the following viewport:

'viewport': { 'width': 414, 'height': 896, 'deviceScaleFactor': 3, 'isMobile': true }

— per the playwright deviceDescriptors file.

For mobile I'll have to set a screensize as well. We could maybe have another *.conf.js for mobile that has that set differently.

I think all that would need to change is the browser.setWindowSize(w, h) dimensions, correct?
Wondering if we could convert that into ENV vars (maybe with a desktop fallback). That way we wouldn't need any new config files and we could possibly hack something like this:

"test:integration:mobile": "BROWSER_WIDTH=414 BROWSER_HEIGHT=896 wdio tests/integration/wdio.conf.js",

Then, above we'd just need to change to this psesudo-code mess:

// NOTE: Env vars come in as strings, iirc, so we cast them to numbers here. browser.setWindowSize( Number(process.env.BROWSER_WIDTH) || 1920, Number(process.env.BROWSER_HEIGHT) || 1080 );

This is good thinking. @groovecoder thoughts on this?

jrbenny35 · 2020-02-19T17:56:08Z

I'm not sure the Breaches_Page baselines will really work in the long run. It seems like any recent breach will cause the items to reorder and we'll need to regen the baselines after each freshly added breach. Possibly multiple times after we upload a new breach logo to the CDN.

Even the Home_Page baseline has a recent breach card. Not sure how viable it would be to hide that DOM element before taking/comparing images, or if that would defeat the purpose and reduce confidence (less brittle test that doesn't test a core thing). man_shrugging

I wonder how exact it is. I think it is worth having but if it comparing words, then yeah, that will be a problem. That is why I wanted to use a range. It calculates a percentage based on how "off" the page is.

pdehaan · 2020-02-19T17:58:46Z

It calculates a percentage based on how "off" the page is.

AAAHHHH... OK, my misunderstanding. The tool we used at a different company measured by pixels so we could only have 0-5 pixels different from a baseline, not 0-5%. I say we run with it for a bit and see how it works in the long run.

jrbenny35 · 2020-02-19T18:09:49Z

It calculates a percentage based on how "off" the page is.

AAAHHHH... OK, my misunderstanding. The tool we used at a different company measured by pixels so we could only have 0-5 pixels different from a baseline, not 0-5%. I say we run with it for a bit and see how it works in the long run.

I agree. If the words make up for 10% or less, I think we are still good (as we discussed on slack).

pdehaan · 2020-02-24T21:45:30Z

From @jrbenny35 in a Slack channel:

there is a new breach added to monitor, the visual tests failed with 6.55% diff

jrbenny35 · 2020-02-24T22:02:07Z

From @jrbenny35 in a Slack channel:

there is a new breach added to monitor, the visual tests failed with 6.55% diff

The homepage failed with a 5.8ish% diff and the "Breaches" page failed with 6.55%. If this keeps gets worse with more breaches, we will have to revist this and maybe create a better baseline.

pdehaan · 2020-02-25T01:08:26Z

If this keeps gets worse with more breaches, we will have to revisit this and maybe create a better baseline.

Yeah, I can imagine the /breaches/ endpoint will be a hot mess(tm). Currently the breaches are sorted by the breach date (which isn't a displayed field, making the ordering appear completely random). So if a breach is new added to the site, it could be displayed first (invalidating the order of all 15 items), or could appear 8th in the list (invalidating only 7-8 of the 15 displayed items), or could be displayed as the 200th most recent breach, which is then hidden behind the "Show All" button, and may not even appear in the image diff (depending on whether you baseline before or after hitting the "Show All" button).

So the fact the "Slickwraps" breach is currently the lastest breach date and date added (putting it in the first slot of the /breaches/ page), seems to be our worst case scenario -- image-diff-wise speaking.

tests/integration/tests/test-home-page.js

jrbenny35 · 2020-03-04T00:02:34Z

If you want to try these locally you will need to follow these instructions:

Copy .env-dist to .env.
Set the following env variables: FXA_ENABLED=1, HIBP_KANON_API_TOKEN=token123, HIBP_API_TOKEN=token123, MONITOR_FXA_PASSWORD=password. Replace the values in "token123" and "password" with the correct ones.
In the same shell instance, start the server and build the docker image docker-compose -f tests/integration/docker-compose.yml up --build -d.
Run the tests: npm run test:integration.

I can provide the MONITOR_FXA_PASSWORD on slack if needed. @groovecoder has it too.

tests/integration/tests/test-create-baseline.js

tests/integration/wdio.conf.js

tests/integration/README.md

pdehaan · 2020-03-05T18:05:45Z

tests/integration/README.md

+3. Run the tests: ```npm run test:integration```.
+   They can also be run headlessly: ```npm run test:integration-headless```, and inside docker: ```npm run test:integration-docker```.
+
+[geckodriver]: https://github.com/mozilla/geckodriver/releases


Ah, interesting! looks like geckodriver is on npm too: https://www.npmjs.com/package/geckodriver (by our very own vladikoff!)

Should I link that instead?

No, sorry, that was just me thinking out loud and bookmarking it for future reference. It'd only make sense to switch if we added it as a project dependency in package.json and got rid of the explicit dependency of having it preinstalled. But I'd have to audit the npm install to see how big that bundle is. In fact, I can do that right now.

Looks like running npm i geckodriver -D in a fresh directory adds about 9.7 MB of node_modules goodness, including the following files in ./node_modules/geckodriver/:

-rwxr-xr-x 5.0M 10 Oct 15:53 geckodriver -rw-r--r-- 1.9M 5 Mar 10:26 geckodriver.tar.gz

But let's just leave this as-is for now and can refactor in the future after this lands.

Good info to have though!

tests/integration/wdio.docker.js

jrbenny35 · 2020-03-12T15:04:30Z

@groovecoder @lesleyjanenorton would love to land this this week

tests/integration/docker-compose.yml

jrbenny35 · 2020-03-20T16:25:22Z

@groovecoder @lesleyjanenorton any updates?

pdehaan · 2020-03-24T20:22:01Z

@groovecoder @lesleyjanenorton Do either of you have any bandwidth to assist with this? We've rebased and are still seeing the test failure on Travis-CI (https://travis-ci.org/github/mozilla/blurts-server/jobs/666489293?utm_medium=notification&utm_source=github_status) but I'm still unclear if it's related to our PR since it doesn't look like we've touched any of the failing code. Although @jrbenny35 rebased and it looks like we should be in sync w/ latest changes to master branch.

I'm at a bit of a loss.

groovecoder

One last change requested. I can't speak much to how we should write all the page tests, so I'm assuming @pdehaan is making the thorough review of that code.

.eslintrc.js

pdehaan · 2020-04-02T15:04:17Z

.env-dist

-OAUTH_CLIENT_ID=cb1bef9d06bb9bc9
-OAUTH_CLIENT_SECRET=f5fb99de6e0af18ab17e013ac1d439903179a97a1c510fc10bc3bd50bbce089b
+OAUTH_CLIENT_ID=edd29a80019d61a1
+OAUTH_CLIENT_SECRET=a80feaad77c847275d39ac989ee12a873ef6b54cbc184128f86f2afecdf003b5


I assume these are OK to change? (I know it's just a dist file, but wanted to confirm these aren't critical creds that need to be regenerated now).

Yup, these are the values I have on my local file and it's working.

pdehaan · 2020-04-02T15:06:38Z

.env-dist

-HIBP_KANON_API_TOKEN=""
+HIBP_KANON_API_TOKEN=
+HIBP_API_ROOT="https://haveibeenpwned.com/api/v2"
+HIBP_API_TOKEN=


Are these OK to be "naked", or should they be empty strings, a la =""?

Yes, according to, https://www.npmjs.com/package/dotenv

pdehaan · 2020-04-02T15:08:14Z

tests/integration/docker-compose.yml

@@ -16,18 +16,40 @@ services:
      - LOGOS_ORIGIN=null
      - BREACH_RESOLUTION_ENABLED=false


Curious, should this be "true"? I think breach resolutions are enabled in production. Curious what effect this would have, apart from the logged in admin being slightly different.

The breach detail pages will also be different for logged-in users.

tests/integration/docker-compose.yml

tests/integration/pages/desktop/scanResults.page.js

tests/integration/wdio.docker.js

pdehaan · 2020-04-02T15:19:41Z

Awesome work @jrbenny35, as always!
Re-reviewed the latest changes and left a few random thoughts and comments.
@groovecoder Can you give this one last 👀 before we merge this into the big code repo in the sky?

groovecoder

Update to eslintrc looks good. Thanks.

jrbenny35 force-pushed the add-initial-integration-tests branch from 6ab9243 to de55c49 Compare February 18, 2020 22:46

jrbenny35 requested review from groovecoder, lesleyjanenorton and pdehaan February 18, 2020 23:21

pdehaan reviewed Feb 18, 2020

View reviewed changes

pdehaan reviewed Feb 19, 2020

View reviewed changes

groovecoder requested changes Feb 19, 2020

View reviewed changes

jrbenny35 requested a review from groovecoder February 25, 2020 16:21

pdehaan reviewed Feb 25, 2020

View reviewed changes

tests/integration/tests/test-home-page.js Outdated Show resolved Hide resolved

jrbenny35 requested a review from pdehaan March 3, 2020 19:47

pdehaan reviewed Mar 4, 2020

View reviewed changes

tests/integration/tests/test-create-baseline.js Outdated Show resolved Hide resolved

pdehaan reviewed Mar 4, 2020

View reviewed changes

tests/integration/wdio.conf.js Outdated Show resolved Hide resolved

pdehaan reviewed Mar 5, 2020

View reviewed changes

lesleyjanenorton reviewed Mar 16, 2020

View reviewed changes

tests/integration/docker-compose.yml Outdated Show resolved Hide resolved

lesleyjanenorton reviewed Mar 16, 2020

View reviewed changes

tests/integration/docker-compose.yml Outdated Show resolved Hide resolved

jrbenny35 force-pushed the add-initial-integration-tests branch from 9d5edb1 to eaad988 Compare March 24, 2020 19:19

groovecoder requested changes Apr 2, 2020

View reviewed changes

.eslintrc.js Show resolved Hide resolved

pdehaan reviewed Apr 2, 2020

View reviewed changes

pdehaan requested a review from groovecoder April 2, 2020 15:18

jrbenny35 added 24 commits April 2, 2020 14:22

Fix typo.:

30f99f3

Remove old docker command.

3da6429

EVERYTHING IN ONE SHELL.

826eab8

Trying nvm.

8ba68c6

Fix typo.

a350fbd

Fix node version...

be9d8c8

Fix typo.

bd76d66

More node stuff.

0143461

Updates to docker compose.

dc7e5cb

Add chown to circleci config.

076d773

Fix docker login error.

9806aae

Change exec command user.

2e8c048

Add screenshots for debugging.

7c5b485

Try fixing js command.

6d8999d

Try fixing js command again.

a43d303

Change screenshot path

814b54f

Add mkdir for error shots.

34cba17

Trying something for postgres.

5eb4399

Updates.

a6ebdd2

Fix lint error.

22d27a0

Try fixing errorshots.

1af3355

Updates before rebase.

ae56c03

Updated baseline for new homepage.

caeb240

Remove baseline image save.

953c541

jrbenny35 force-pushed the add-initial-integration-tests branch from f595c7a to 953c541 Compare April 2, 2020 20:01

jrbenny35 added 2 commits April 2, 2020 15:18

Trying a change for errorshots.

d04ad2f

Update test.

69336c1

groovecoder approved these changes Apr 3, 2020

View reviewed changes

jrbenny35 merged commit b19eff6 into mozilla:master Apr 3, 2020

jrbenny35 mentioned this pull request Apr 3, 2020

Automated test suite #1556

Closed

		@@ -16,18 +16,40 @@ services:
		- LOGOS_ORIGIN=null
		- BREACH_RESOLUTION_ENABLED=false

Initial commit adding integration tests. #1557

Initial commit adding integration tests. #1557

Conversation

jrbenny35 commented Feb 18, 2020

pdehaan left a 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

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

This comment was marked as resolved.

Choose a reason for hiding this comment

pdehaan left a 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

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

This comment was marked as resolved.

This comment was marked as resolved.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrbenny35 commented Feb 19, 2020

pdehaan commented Feb 19, 2020

jrbenny35 commented Feb 19, 2020

pdehaan commented Feb 24, 2020

jrbenny35 commented Feb 24, 2020 • edited Loading

pdehaan commented Feb 25, 2020

jrbenny35 commented Mar 4, 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

jrbenny35 commented Mar 12, 2020

jrbenny35 commented Mar 20, 2020

pdehaan commented Mar 24, 2020

groovecoder left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdehaan commented Apr 2, 2020

groovecoder left a comment

Choose a reason for hiding this comment

jrbenny35 commented Feb 24, 2020 •

edited

Loading

jrbenny35 commented Mar 4, 2020 •

edited

Loading