Local visreg #60

Droogans · 2017-10-12T20:49:24Z

JIRA: https://jira.rax.io/browse/SURF-616

Not sure if a Design LGTM is called for here but I can at least get a demo put together for designers during a zoom review.

You can run this by navigating into the test directory, running yarn install, then npm run visreg {branchName}.

I'll demo it in better detail during the zoom review.

LGTM's

Dev LGTM
Zoom LGTM

Droogans · 2017-10-13T13:39:46Z

I'm going to keep dumping more and more review related commits here, and try to never force push.

When it looks good to squash, I'll close this PR and open local-visreg-squashed.

Droogans · 2017-10-16T19:37:18Z

At a minimum draw a picture (architecture diagram) of the different pieces and what kinds of input are required/used.

catsiller

Zoom LGTM

Droogans

From the PR review yesterday.

Droogans · 2017-10-16T19:15:35Z

test/bin/visreg.example.config.ts

@@ -0,0 +1,13 @@
+export interface IConfig {


Comment on all of this stuff.

Including the fact that this isn't run because it needs to be copied first.

Droogans · 2017-10-16T19:20:55Z

test/bin/visreg.ts

+
+    const storedToken: any = fs.existsSync(f) && fs.readFileSync(f);
+    const token = (storedToken || (await password("github PAT: ")));
+    const repoUrl = url.parse(`https://github.rackspace.com/${config.githubName}/${config.repo}`);


Use the config here.

Droogans · 2017-10-16T19:23:59Z

test/bin/visreg.ts

+            `cd ${config.screenshotsDirectory}`,
+            `git add -A`,
+            `git status -sb`,
+            `git commit -m "Baseline"`,


Better commit message?

Droogans · 2017-10-16T19:30:40Z

test/index.ts

+        });
+    });
+
+    describe.skip("firefox", () => {


Droogans · 2017-10-16T19:32:57Z

test/scripts/webdriver-start

@@ -0,0 +1,6 @@
+#!/usr/bin/env bash
+
+java -Dwebdriver.gecko.driver=bin/selenium/geckodriver \


Document this.

Droogans · 2017-10-16T19:33:10Z

test/scripts/webdriver-update

+
+wget -nc -q https://{GH_TOKEN}@github.com/mozilla/geckodriver/releases/download/v0.18.0/geckodriver-v0.18.0-${platform_gecko}.tar.gz -O bin/selenium/geckodriver-v0.18.0.tar.gz
+wget -nc -q https://chromedriver.storage.googleapis.com/2.31/chromedriver_${platform_chrome}.zip -O bin/selenium/chromedriver_2.31.zip
+wget -nc -q https://selenium-release.storage.googleapis.com/3.4/selenium-server-standalone-3.4.0.jar -O bin/selenium/selenium-server-standalone.jar


Document this too (brew install wget).

Droogans · 2017-10-16T19:38:32Z

test/bin/visreg.example.config.ts

+
+export var config: IConfig = {
+    "githubEmail": "andrew.yurisich@rackspace.com",
+    "githubName": "andr6283",


This needs a github hostname.

Also, consider removing my personal information and instead leave it blank or use an obviously fake account for info.

Droogans · 2017-10-16T19:40:51Z

test/bin/visreg.ts

+        ];
+        return safeExecSync(cmds.join('; '));
+
+    };


All of these anonymous util functions can be brought out into it's own file and exported.

Droogans · 2017-10-19T17:13:37Z

https://github.com/rackerlabs/helix-ui/wiki/Testing updated

Looking to close this and open the squashed version of this. Should I be squashing this down to just one commit, @CITguy?

CITguy · 2017-10-19T19:19:32Z

test/bin/_visreg.ts

@@ -0,0 +1,232 @@
+/*


Rename to util.ts to match common javascript module naming convention.

CITguy · 2017-10-19T19:26:06Z

test/scripts/webdriver-update

+#!/usr/bin/env bash
+
+# https://stackoverflow.com/a/4785518/881224
+which wget >/dev/null 2>&1 || { echo "Install wget. Try 'brew install wget' on mac. Aborting." >&2; exit 1; }


Generalize the message to be less system-specific.

CITguy · 2017-10-19T19:26:46Z

test/scripts/webdriver-update

+wget -nc -q https://selenium-release.storage.googleapis.com/${TARFILE_STANDALONE} -O bin/selenium/selenium-server-standalone.jar
+tar -xzf bin/selenium/geckodriver-v0.18.0.tar.gz -C bin/selenium/
+unzip -o bin/selenium/chromedriver_2.31.zip -d bin/selenium/
+cd node_modules/.bin/ && ln -s ../../bin/selenium/* .


Add comment about why this is needed.

Will it be a problem if things are already symlinked?

No because the script starts by removing them first.

CITguy · 2017-10-19T19:34:44Z

test/bin/_visreg.ts

+export const repoUrl = url.parse(`https://${config.githubHostname}/${config.githubName}/${config.repo}`);
+export const hasCommitRegex = /\[.*([0-9a-f]{7})] Checking in screenshots.../;
+
+export async function getBranchName(targetBranch: string) {


maybe rename to askBranch?

CITguy · 2017-10-19T19:36:28Z

test/bin/_visreg.ts

+    return branch;
+}
+
+export async function getGithubToken() {


maybe rename to askGithubToken?

CITguy · 2017-10-19T19:40:08Z

test/bin/_visreg.ts

+    }
+
+    if (unknownError) {
+        throw new Error("Something has gone very wrong " + afterCommitData);


Why no template literal?

CITguy · 2017-10-19T19:41:09Z

test/bin/_visreg.ts

+};
+
+export function buildCurlFlags(token: string) {
+    const flags = [


Can you use a multi-line template literal?

CITguy · 2017-10-19T19:42:32Z

test/bin/_visreg.ts

+            res.on("data", d => { data.push(d.toString())});
+            if (res.statusCode !== 201) {
+                res.on("end", () => {
+                    const msg = `(HTTP ${res.statusCode}) Something went wrong while creating the repository ${repoUrl.href}:\n${data.join("")}`;


This might be useful as a multi-line template literal.

CITguy · 2017-10-19T19:43:37Z

test/bin/_visreg.ts

+};
+
+function commitScreenshots(token: string, screenshotsDirectory: string) {
+    let cmds = [


Can this be a multi-line template literal?

CITguy · 2017-10-19T19:46:21Z

test/scripts/webdriver-start

+
+# https://stackoverflow.com/a/4785518/881224
+LINK="https://github.com/rackerlabs/helix-ui/wiki/Testing#java"
+which java >/dev/null 2>&1 || { echo ${LINK}; echo "Install java. Try 'brew install java' on mac. Aborting." >&2; exit 1; }


Generalize the message to be less system-specific.

CITguy

Dev LGTM

We know it's a work in progress, but this looks good enough for the first iteration.

Droogans requested review from CITguy and catsiller October 12, 2017 20:49

CITguy added the PR: Needs Squash label Oct 12, 2017

catsiller approved these changes Oct 16, 2017

View reviewed changes

Droogans commented Oct 17, 2017

View reviewed changes

Droogans added 23 commits October 19, 2017 11:30

chore(ts): Copy setup from J.

e0cf28f

test(wip): More updates for J.

baf3873

fix(npm): test command not 'and'ed

d424540

test(helix): Shorten failing example

8b63f54

chore(npm): add clean task

77612f2

chore(gitignore): Screenshots directory

76eb300

chore(mocha): Don't --bail anymore

5a042d5

test(setup): Try a new shortcut out

999ebbf

chore(cleaup): Make mocha run without opts file

a793f51

chore(npm): add clean task

c7839bd

chore(yarn): Upgrade visreg tool

00f7efc

refactor(setup): Move into before

61cec8d

test(visreg): Use default function for xpect

5799668

test(visreg): Default error fallback for new screenshots

cfae542

fix(errors): Less verbose

e596e0a

fix(errors): Throw on any

d317c58

chore(visreg): Avoid new code with upgrade to 0.4.2

b93c6d8

test(visreg): Resize browser to 1366x768

a39da3f

test(cropping): Account for retina

3311b31

test(visreg): Nav Guides section

86bdde3

refact(test): Move directory from scripts to test

f0aafeb

test(helix): For chrome and ff

8c17fab

spike(visreg): CLI tool wip

4a3402b

Droogans added 12 commits October 19, 2017 11:30

test(visreg): Remove another unused value from config

55a1322

test(visreg): Always start baselines away from anything

7e7ce23

fix(visreg): Detect changed screenshot remote

99b4988

fix(visreg): Better default install behavior

eef894a

fix(visreg): Handle bad argument handling

30fcf44

fix(visreg): Better vpn connection warning message

35b17b2

style(visreg): Make prompts match

6aeb4ba

refact(visreg): Split out util helpers

a26e3e9

style(visreg): Better diff url info printed to console

ef6df8c

chore(visreg): Unskip firefox

58515ba

fix(visreg): Allow for firefox usage

cf1ee4b

fix(visreg): Update selectors

7318eef

Droogans force-pushed the local-visreg branch from 329a642 to 7318eef Compare October 19, 2017 15:46

docs(visreg): Link to java install help

7d14b01

Droogans force-pushed the local-visreg branch from 029a500 to 7d14b01 Compare October 19, 2017 17:21

CITguy suggested changes Oct 19, 2017

View reviewed changes

Droogans added 3 commits October 20, 2017 09:43

test(visreg): Update config automatically

94ab515

fix(visreg): Rename _visreg file to utils.ts

e038fba

test(visreg): Update config automatically

bb0b3c2

Droogans force-pushed the local-visreg branch from 9863439 to bb0b3c2 Compare October 20, 2017 15:24

Droogans added 5 commits October 20, 2017 11:29

fix(visreg): Account for before/after in commit messages

72df2af

docs(visreg): Generalize install warning errors

7e237b7

docs(visreg): Explain use of symlinks

84f17fb

style(visreg): Avoid unneeded variable shortening

3605595

fix(visreg): Move test helpers out into util file

cb0ed05

CITguy approved these changes Oct 20, 2017

View reviewed changes

Droogans mentioned this pull request Oct 20, 2017

test(visreg): Local visreg service #65

Merged

3 tasks

Droogans closed this Oct 20, 2017

CITguy deleted the local-visreg branch March 7, 2018 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local visreg #60

Local visreg #60

Droogans commented Oct 12, 2017 •

edited by CITguy

Loading

Droogans commented Oct 13, 2017

Droogans commented Oct 16, 2017

catsiller left a comment

Droogans left a comment

Droogans Oct 16, 2017

Droogans Oct 16, 2017

Droogans Oct 18, 2017 •

edited

Loading

Droogans Oct 16, 2017

Droogans Oct 16, 2017

Droogans Oct 16, 2017

Droogans Oct 16, 2017

Droogans Oct 16, 2017

Droogans Oct 16, 2017

Droogans Oct 16, 2017

Droogans Oct 16, 2017

Droogans commented Oct 19, 2017 •

edited

Loading

CITguy Oct 19, 2017

CITguy Oct 19, 2017

CITguy Oct 19, 2017

CITguy Oct 19, 2017

Droogans Oct 20, 2017

CITguy Oct 19, 2017

CITguy Oct 19, 2017

CITguy Oct 19, 2017

CITguy Oct 19, 2017

CITguy Oct 19, 2017

CITguy Oct 19, 2017

CITguy Oct 19, 2017

CITguy left a comment

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

		java -Dwebdriver.gecko.driver=bin/selenium/geckodriver \

Local visreg #60

Local visreg #60

Conversation

Droogans commented Oct 12, 2017 • edited by CITguy Loading

LGTM's

Droogans commented Oct 13, 2017

Droogans commented Oct 16, 2017

catsiller left a comment

Choose a reason for hiding this comment

Droogans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Droogans Oct 18, 2017 • 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

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

Droogans commented Oct 19, 2017 • 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

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

CITguy left a comment

Choose a reason for hiding this comment

Droogans commented Oct 12, 2017 •

edited by CITguy

Loading

Droogans Oct 18, 2017 •

edited

Loading

Droogans commented Oct 19, 2017 •

edited

Loading