Smoke test that runs in travis #747

KenCoder · 2018-04-19T18:49:51Z

Don't merge yet - I intentionally put a console.error statement to see if this fails travis on the PR build.

garris · 2018-04-19T20:50:18Z

package.json

@@ -21,7 +21,7 @@
    "build-compare": "webpack --config ./compare/webpack.config.js",
    "dev-compare": "webpack-dev-server --content-base ./compare/output --config ./compare/webpack.config.js",
    "integration-test": "rm -rf newdir && mkdir newdir && cd newdir && node ../cli/index.js genConfig && node ../cli/index.js reference && node ../cli/index.js test && node -e \"require('../')('test')\"",
-    "smoke-test": "cd test/configs/; node ../../cli/index.js test --config=backstop_features",
+    "smoke-test": "node test/smoke/smoke_test.js",


Very nice harness you built there!

A suggestion: would you consider leaving the smoke-test command the same (so it executes the backstop_features test like before) -- and add a new command for your new harness. Maybe call it like, travis-test or git-commit-test or deploy-test?

I am totally open to suggestions -- my instinct is to keep it convenient for devs to still be able to run the raw smoke-test and see results (passing or non-passing).

Does this make sense?

I think the harness works well for devs too because it tells them if it succeeds at the end. Right now it's hard to notice if something breaks (e.g., incorrect # of comparison failures or that a timeout). There are some scary looking messages in the output that I don't know if they mean somethings wrong or not (selector not found, etc).

The only difference is that the CI test doesn't pop up the browser report at the end.

But I will add an argument so if you do
npm run smoke-test

It will behave just like today with additional niceness and popup the browser, and

npm run smoke-test -- ci

Will do it without the browser report.

Question for you: Will this have to run in Docker? It seems like launching in travis doesn't really work. If so I can't currently see the docker cloud config - maybe you can share it with me?

Sounds good!

I honestly am not an expert in docker -- I just know enough to get me into trouble. I am happy to share the configs with you -- they should all be in the /docker directory.

Also -- just so I am understanding correctly -- when you say...

It seems like launching in travis doesn't really work

...are you saying you cant run backstop tests in travis?

It seems like launching in travis doesn't really work

Well that's the way it is now - you can click below and see the error. I wasn't sure if you already knew this. If not - no worries I think I can fix this problem. So expect an updated PR shortly.

garris

Thank you for working on all of this! I am really really under the gun at work — it could take a little bit for me to test/merge this. It will happen ASAP! Thanks for your patience. Cheers.

KenCoder · 2018-04-24T01:43:40Z

Don't merge yet! I'm still working on it - unfortunately chrome in Linux produces image mismatch with chrome on the mac, so likely the mac version will have to use a docker container to generate the reference bitmaps. I will let you know when it's ready, and no worries about work - I know how it is.

garris · 2018-04-24T01:58:01Z

😄 ok thank you.

BTW: if you are running into rendering differences you could try adding this to the settings...

    "args": [
      "--no-sandbox",
      "--disable-setuid-sandbox",
      "--disable-gpu", 
      "--force-device-scale-factor=1", 
      "--disable-infobars=true"
    ]

In particular, —disable-gpu when used on Mac and Linux may fix the font rendering issues. It does seem to work across macOS boxes and may work across Linux and Mac.

It would be good to know if this in fact can solve the issue. I am happy to rerender the images using that setting if you don’t have a Mac handy.

And again, thanks so much for your effort 😊.

KenCoder · 2018-04-27T17:13:14Z

assets/styles.css

@@ -43,7 +43,6 @@ a:hover {
  transform: translateZ(200px);
 }
 .lemurInYourFace {
-  transition: transform 750ms ease, opacity 750ms ease, visibility 800ms ease;


This line was cuasing intermittent failures in the backstop tests. Since the lemurInYourFace is part of the initial styling with the index.html, the fade-in would happen on page load and therefore you'd occasionally get lemurs partially loaded especially in the noDelay scenario

Of course in real life -- our designer would not let us remove a style because it was inconvenient to test 😄. Can we modify the test if this is causing a failure?

The way to do this is with the transitionend event. This could be a new backstop feature, or one can write custom Javascript to detect the transition and use the readyEvent feature of Backstop. There is already a test of readyEvent - this 'lemurInYourFace' is supposed to be the 'Simple' test case. Demonstrating CSS transition in the "Simple" case seems wrong.

How about we add another issue "demo CSS transition handling" that can be done in a separate PR? I didn't introduce this problem - I'm just cleaning up something that was already flaky.

yes that works. We can just comment the flaky test now and create an issue.

There is actually an article on dwb which discusses exactly your idea — using the transition event. The onReady test script can attach a listener and just emit a console log which triggers the shot.

KenCoder · 2018-04-27T17:19:35Z

Ok I'm ready! Here's what I did:

I created two folders: linux and osx, each with a backstop_data folder under it
I moved the current backstop_data/bitmaps_reference to osx/backstop_data
When you run a regular smoke test, it figures out which machine type you are on and uses that folder. For most people, doing this locally means running on a mac
If you run "npm smoke-test -- ci" on the mac, after it runs the mac smoke tests, it will build and run a docker image to check / update the smoke tests that will run in travis. This is how I populated linux/backstop_data/bitmaps_reference
In travis, it uses the linux backstop data images
The container mounts the current folder to /host-backstop inside docker. There is some magic to isolate the node_modules container inside the container from the one on the mac, since they load different stuff (e.g., the binary version of chrome)
the first time you use the container on the mac, it takes a while to build the container. After that it mostly reuses stuff.

KenCoder · 2018-05-07T18:52:55Z

Hey - any luck reviewing this? Any concerns?

garris · 2018-05-08T02:01:38Z

Oh crap — sorry man. So busy... 😿

I have a day to work on the library on 5/18 — but hopefully I can this before that.

Thanks for your patience!

KenCoder · 2018-05-08T10:21:02Z

No worries, not in any big hurry, I just wanted to make sure it was in the queue.

garris · 2018-05-18T17:23:27Z

assets/styles.css

@@ -43,7 +43,6 @@ a:hover {
  transform: translateZ(200px);
 }
 .lemurInYourFace {
-  transition: transform 750ms ease, opacity 750ms ease, visibility 800ms ease;


Of course in real life -- our designer would not let us remove a style because it was inconvenient to test 😄. Can we modify the test if this is causing a failure?

garris · 2018-05-19T05:02:22Z

test/configs/backstop_features.js

 const ENGINE = 'puppet';
 const SCRIPT_PATH = 'puppet';
+const PLATFORM = process.env.BACKSTOP_SMOKE_PLATFORM || (process.platform === 'darwin' ? 'osx' : 'linux');

 module.exports = {
  id: `${ENGINE}_backstop_features`,


The ID property is appended to all test images -- so adding your platform parameter there enables backstop to track these resources at file level.

Line 8 can be

id: `${PLATFORM}_${ENGINE}_backstop_features`,

Yes this is much nicer - I changed it to this.

garris · 2018-05-19T05:26:13Z

test/configs/backstop_features.js

@@ -83,7 +86,7 @@ module.exports = {
    },
    {
      label: 'cookies',
-      cookiePath: 'backstop_data/cookies.json',
+      cookiePath: `${PLATFORM}/backstop_data/cookies.json`,


I noticed that you are not using cookies -- so in this case it seems unnecessary to customize the cookie path. And even if we did have cookies -- it's likely these would share the same file so the original (default) value does not need to be changed here.

garris · 2018-05-19T05:28:59Z

test/configs/backstop_features.js

-    engine_scripts: 'backstop_data/engine_scripts',
-    html_report: 'backstop_data/html_report',
-    ci_report: 'backstop_data/ci_report'
+    bitmaps_reference: `${PLATFORM}/backstop_data/bitmaps_reference`,


Reference and test scripts can all remain using their default values.

garris · 2018-05-19T05:29:36Z

test/configs/backstop_features.js

-    ci_report: 'backstop_data/ci_report'
+    bitmaps_reference: `${PLATFORM}/backstop_data/bitmaps_reference`,
+    bitmaps_test: `${PLATFORM}/backstop_data/bitmaps_test`,
+    engine_scripts: `engine_scripts`,


Engine scripts should stay in the backstop_data directory. (keep default value)

garris · 2018-05-19T06:40:23Z

test/smoke/docker_smoke.sh

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


I would love to know what each of these lines did. I find it hard to parse docker configs. Particularly lines 5 & 6. I guess I expect we would cd to a workspace, install backstop locally to that space and use the feature tests runner. I don't understand why /test/configs/linux/node_modules is called out in there.

garris · 2018-05-19T06:45:12Z

test/smoke/smoke_test.js

+    { cwd: configsDir, env: processEnv }
+  );
+
+  const requiredStdout = [


This sort of thing should be placed at the top of this script with explicit comments around how to use this array to handle different test scenarios.

In this case we should name it something very clear.

garris · 2018-05-19T06:45:29Z

test/smoke/smoke_test.js

+    'report | 0 Failed'
+  ];
+
+  const allowedStdErr = [


same comment as above for this array.

garris · 2018-05-19T06:46:04Z

test/smoke/smoke_test.js

+
+  let missingStdOut = [...requiredStdout];
+
+  child.stdout.on('data', function (buf) {


Please use named functions for clarity.

garris · 2018-05-19T06:49:25Z

test/smoke/smoke_test.js

+
+  const code = await new Promise(resolve => child.on('close', resolve));
+
+  const problems = [];


would prefer if these were encapsulated a bit by functions so that the overall flow of this script can be easily grasped in a few lines.

garris

Hi Ken -- thank you very much for all your effort here! This is great work. I do have some comments though. I think this implementation can be significantly tightened up such that it would serve a nice reference implementation for other developers.

I'd like to take a more backstop-centric approach to the layout -- this would improve developer ergonomics and make the system easier to reason about. I called out suggestions which would enable you to allow backstop to manage all resources in it's own scaffolding (as intended by design). Instead of separate directories, images should be tagged 'linux' or 'osx' and managed by backstop in bitmap_reference. Most everything else is platform agnostic so separate platform directories are not necessary.

Let me know what you think.

garris

Thanks for making the changes.

I will pull these changes directly from your branch and test on 6/15.

I’ll put the css line back and also take care of line 131.

Thanks for all your work on this — it’s awesome.

Best
Garris

garris · 2018-05-26T04:28:12Z

test/configs/backstop_features.js

@@ -125,15 +128,16 @@ module.exports = {
  paths: {
    bitmaps_reference: 'backstop_data/bitmaps_reference',
    bitmaps_test: 'backstop_data/bitmaps_test',
-    engine_scripts: 'backstop_data/engine_scripts',
+    engine_scripts: 'engine_scripts',


Should also be under backstop data.

gabegorelick · 2018-09-26T16:05:23Z

Is there anything preventing this from being merged, aside from needing a rebase? More tests would give contributors a lot of confidence their changes won't break things.

KenCoder · 2018-09-27T12:05:35Z

We need to address #842 for the smoke tests to pass consistently. Then it needs to rebase.

Smoke test that runs in travis

6215051

garris reviewed Apr 19, 2018

View reviewed changes

KenCoder added 6 commits April 20, 2018 16:22

Seeing if no-sandbox will fail in Travis

37b47cf

Continue tweaking to get smoke test failing in travis

41c8d88

Continue updating & testing for travis

c0a5dec

Merge remote-tracking branch 'backstop/master' into smoke-in-travis

b141a10

Should be working, except the noDelay test is somewhat flaky

c85ade9

Try some unix settings to align fonts

a67273c

garris reviewed Apr 23, 2018

View reviewed changes

KenCoder added 3 commits April 27, 2018 09:13

Separate smoke tests for Linux and Mac

7cafd31

No object spread

fd9edfb

Revert some unintentional changes

0dbb035

KenCoder commented Apr 27, 2018

View reviewed changes

garris requested changes May 19, 2018

View reviewed changes

Use Backstop id to separate Linux/OSX

0d93a90

garris reviewed May 26, 2018

View reviewed changes

KenCoder added 2 commits August 16, 2018 13:33

I don't remember where I left this - committing to set aside

2bfec1c

Finished rework

8deb3bf

KenCoder mentioned this pull request Aug 23, 2018

Smoke tests are flaky because of CSS transition #842

Open

brendonbribeiro mentioned this pull request Nov 13, 2018

[WIP] Smoke tests with docker #918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smoke test that runs in travis #747

Smoke test that runs in travis #747

KenCoder commented Apr 19, 2018

garris Apr 19, 2018

KenCoder Apr 19, 2018

garris Apr 19, 2018 •

edited

Loading

KenCoder Apr 20, 2018

garris left a comment

KenCoder commented Apr 24, 2018

garris commented Apr 24, 2018 •

edited

Loading

KenCoder Apr 27, 2018

garris May 18, 2018

KenCoder May 19, 2018

garris May 19, 2018

KenCoder commented Apr 27, 2018 •

edited

Loading

KenCoder commented May 7, 2018

garris commented May 8, 2018

KenCoder commented May 8, 2018

garris May 18, 2018

garris May 19, 2018

KenCoder May 22, 2018

garris May 19, 2018

KenCoder May 22, 2018

garris May 19, 2018

KenCoder May 22, 2018

garris May 19, 2018

garris May 19, 2018

garris May 19, 2018

garris May 19, 2018

garris May 19, 2018

garris May 19, 2018

garris left a comment •

edited

Loading

garris left a comment •

edited

Loading

garris May 26, 2018

gabegorelick commented Sep 26, 2018

KenCoder commented Sep 27, 2018


		let missingStdOut = [...requiredStdout];

		child.stdout.on('data', function (buf) {


		const code = await new Promise(resolve => child.on('close', resolve));

		const problems = [];

Smoke test that runs in travis #747

Are you sure you want to change the base?

Smoke test that runs in travis #747

Conversation

KenCoder commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garris Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garris left a comment

Choose a reason for hiding this comment

KenCoder commented Apr 24, 2018

garris commented Apr 24, 2018 • 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

KenCoder commented Apr 27, 2018 • edited Loading

KenCoder commented May 7, 2018

garris commented May 8, 2018

KenCoder commented May 8, 2018

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

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

garris left a comment • edited Loading

Choose a reason for hiding this comment

garris left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabegorelick commented Sep 26, 2018

KenCoder commented Sep 27, 2018

garris Apr 19, 2018 •

edited

Loading

garris commented Apr 24, 2018 •

edited

Loading

KenCoder commented Apr 27, 2018 •

edited

Loading

garris left a comment •

edited

Loading

garris left a comment •

edited

Loading