Epic7 e2e tests #1413

dan-f · 2017-10-12T14:59:12Z

This one fixes #1316.

I figured the benefits of driving multiple browsers concurrently (not yet introduced here) outweighs the nicer API cypress offers.

mattkrick · 2017-10-12T15:08:59Z

package.json

@@ -14,35 +14,35 @@
  },
  "main": "index.js",
  "scripts": {
-    "start": "NODE_ENV=production node --trace-warnings ./src/server/server.babel.js",
+    "start": "NODE_ENV=${NODE_ENV:-production} node --trace-warnings ./src/server/server.babel.js",


what's this do?

If there's a NODE_ENV already bound in the environment, it uses that value, otherwise production.

can you give me an example where this is useful? it kinda scares me because this makes it seem a little less expressive because after calling this, i still don't know what env it's running in.

I could revert this change if you like - it's a holdover from when I was associating the value of NODE_ENV with the .env.<NODE_ENV> files.

That said, here's an example of how it'd work:

$ # when NODE_ENV='' (unbound) $ npm start # NODE_ENV is bound to 'production' $ # when NODE_ENV='production' $ npm start # NODE_ENV is bound to 'production' $ # when NODE_ENV='development' $ npm start # NODE_ENV is bound to 'development' $ # ...etc

codecov · 2017-10-12T15:25:09Z

Codecov Report

Merging #1413 into epic7 will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            epic7    #1413   +/-   ##
=======================================
  Coverage   44.01%   44.01%           
=======================================
  Files         359      359           
  Lines        4194     4194           
  Branches      572      572           
=======================================
  Hits         1846     1846           
  Misses       2019     2019           
  Partials      329      329

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91217a9...880bc05. Read the comment docs.

dan-f · 2017-10-13T15:24:28Z

package.json

    "start:tunnel": "export $(grep ^ULTRAHOOK_API_KEY .env | tr -d '\"') && ultrahook -k $ULTRAHOOK_API_KEY dev 3000",
-    "dev": "if [ ! -f \"dll/vendors.json\" ] ; then npm run build:dll ; fi ; NODE_ENV=development node --trace-warnings ./src/server/server.babel.js",


[FYI] - most of these changes are just me re-ordering things in rough alphabetical order / trying to keep similar scripts close to each other.

dan-f · 2017-10-13T18:56:18Z

Hey @mattkrick - if you have any early feedback, please feel free to leave it! I've got the auth tests working, and I'm planning to refactor that runTests script a bit so that it's a bit less monolithic / can run tests against an existing server, rather than always spinning one up on it's own.

I'm also thinking of keeping this PR scoped to a proof-of-concept implementation, and adding more e2e tests incrementally. If you hav other feelings let me know!

dan-f · 2017-10-13T18:59:33Z

src/universal/utils/dotenv.js

+  const config = {silent: true};
+
+  if (NODE_ENV) {
+    config.path = `.env.${NODE_ENV}`;


I think this change will have implications for others' dev environments, so I can revert it if need be. The main point is that this used to only use .env.test when NODE_ENV === "test", and I changed it to use .env.${whateverTheNodeEnvIs}. I'm finding it useful to keep a bunch of local .env files for different builds.

i'm not a huge fan of the multiple .env files. it may be a necessary evil for running a test on CI, but i'd be more inclined to get rid of the .env.test and move that stuff to the CI internal environment instead of expanding the scope. see https://github.com/motdotla/dotenv#should-i-have-multiple-env-files

I think this is fine, as long as .env* is in .gitignore. It's a fine thing to be able to be able to run your local instance and fire up the e2e tests for local integration (or even local continuous integration). I'd also suggest a fallback to .env if it cannot find the local .env.$NODE_ENV file...

jordanh · 2017-10-13T19:36:05Z

e2e/runTests.js

+const execP = promisify(child_process.exec);
+const globP = promisify(glob);
+
+const SERVER_TIME_TO_START = 10000;


I'd suggest that any of these "big knob" constants are kept in their own file.

Yessir! I'd love to be able to fork the server process and receive a message back when the server is ready to accept connections. If you've got any ideas, let me know! I'll be looking into this in my current refactor.

jordanh · 2017-10-13T19:36:30Z

e2e/runTests.js

+    const mocha = new Mocha({
+      // Note that 20 seconds is a reasonably long timeout.  I'm betting that
+      // tests will generally fail due to errors rather than timeouts.
+      timeout: 20000,


This is also a "big knob" constant, and should be considered to be in its own file

Webdriver and server working!

dan-f · 2017-10-17T20:07:14Z

@jordanh @mattkrick - if either of you'd like to take a final look, I've addressed feedback and got the initial tests running on circle! I'm thinking that further testing can be added on incrementally rather than monolithically in this PR.

mattkrick

absolutely amazing! i really tried to beat this up but all i could find were a couple nits. really, really great.

mattkrick · 2017-10-18T02:31:39Z

circle.yml

+      - type: shell
+        name: E2E test build
+        command: |
+          cp $DEVOPS_WORKDIR/environments/e2e ./.env


mattkrick · 2017-10-18T02:33:58Z

e2e/scripts/drivers.js

+
+export async function hasDrivers() {
+  try {
+    const realDriverFilenames = new Set(await promisify(fs.readdir)(DRIVERS_DIR));


woooo this is a lot to take in for a single line, i'm cool with it if you are though

Haha, yeah, I guess it is a bit much. I'll leave it for now; not expecting this file to cause a lot of tension.

mattkrick · 2017-10-18T02:40:46Z

e2e/scripts/drivers.js

+export async function hasDrivers() {
+  try {
+    const realDriverFilenames = new Set(await promisify(fs.readdir)(DRIVERS_DIR));
+    return !DRIVERS.find((driver) => !realDriverFilenames.has(driver.driverFilename));


my brain is a little fried, but couldn't you simplify away the double negative?

I'm always on the lookout to good solutions to brain fry!

I believe the double negative here is necessary, because if you think about it, it's not a real double negative:

The inner !realDriverFilenames.has(driver.driverFilename): We iterate over the DRIVERS array, looking for one that is not downloaded. If we got rid of this, we might not make it through the whole array, and may miss a missing driver.

The outer !DRIVERS.find(...): if that inner find returns us a driver, then we need to download that webdriver; if it returns undefined, then we've got all our drivers. The truthiness of those values is the opposite of the question "do we have all of our drivers?"

Hope that helps!

mattkrick · 2017-10-18T02:47:06Z

e2e/scripts/ensureDrivers.js

+}
+
+async function ensureDrivers() {
+  const drivers = [


could we replace this with the DRIVERS constant? to dry it up?

Doh! Yes! Good catch.

mattkrick · 2017-10-18T02:48:55Z

e2e/scripts/ensureDrivers.js

+  }
+}
+
+async function ensureDrivers() {


you could make this a sync function & just return Promise.all if you wanted. Usually eslint picks up things that don't need to be async.

Good catch!

mattkrick · 2017-10-18T02:49:59Z

e2e/scripts/runTests.js

+
+commander
+  .version('1.0.0')
+  .description('Runs end-to-end tests against the Action app')


heh, let's call it Parabol. Calling it Action is deprecated, but hard to move away from 😄

You got it!

mattkrick · 2017-10-18T02:55:35Z

e2e/tests/authentication.test.js

+      .click();
+    await all(
+      driver
+        .wait(until.urlMatches(BASE_URL_REGEX), 2000, 'Logging out did not redirect to the base URL'),


can we make that 2000 a constant? i can see us reusing this a bunch
Maybe 2000 for things that relay on our server. 5000 for things that relay on a 3rd party like auth0, etc.

Yeah, fair point. My only tension on this is that timing is finicky, and some things may take longer than others. That said, we can address that when we get there. What if I declared a wait object that looks like { short: 2000, medium: 3000, long: 5000 }?

i like it! and maybe a comment to say when to use each. it'll be a heuristic, but at least it'll give the next guy an idea of where to start.

mattkrick · 2017-10-18T02:59:40Z

e2e/tests/authentication.test.js

+    cache.credentials = credentials;
+  });
+
+  it('can log in (and out) with valid credentials', async () => {


this makes me so happy i could barf. well done!

Gee, thanks!

mattkrick · 2017-10-18T03:01:32Z

package.json

@@ -14,35 +14,35 @@
  },
  "main": "index.js",
  "scripts": {
-    "start": "NODE_ENV=production node --trace-warnings ./src/server/server.babel.js",
+    "start": "NODE_ENV=${NODE_ENV:-production} node --trace-warnings ./src/server/server.babel.js",


can you give me an example where this is useful? it kinda scares me because this makes it seem a little less expressive because after calling this, i still don't know what env it's running in.

mattkrick · 2017-10-18T03:23:02Z

@dan-f great work! Only bug is that it borks the build on deployment.
The error is Error: Cannot find module '../../build/assets.json'.
That makes me think that the build directory isn't being built properly.
perhaps the {NODE_ENV:-production} isn't setting it to production? when we run npm run build?
if that's the case, we could always have a build:e2e that explicitly sets the env to test.

dan-f · 2017-10-18T13:41:25Z

Hey @mattkrick thanks for the suggestions and kind words! Regarding your last comment (Error: Cannot find module '../../build/assets.json')

Oops! I see two patterns here:

Revert that {NODE_ENV:-production} change; every npm script uses the exact NODE_ENV it's declared to use.
- Pros: Scripts rely on less global state; are predictable
- Cons: We'll end up with some duplicated scripts; e.g. npm run db:migrate vs npm run db:migrate-e2e; If we were to do this I'd like to see an enforced pattern for consistency.
Keep that change; npm scripts that care about NODE_ENV need to be called with an explicit NODE_ENV
- Pros: need to maintain fewer scripts; script names are just about what they do and not about where they do it; the NODE_ENV is explicit from how scripts are called
- Cons: need to explicitly declare NODE_ENV in CI/CD configs

So we could fix this by either reverting the change such that npm run build always uses NODE_ENV=production, or we could just call NODE_ENV=production npm run build in the CI config.

Looking forward to your thoughts!

mattkrick · 2017-10-18T14:28:21Z

IMO i like the first option, but i almost always opt for a bunch of smaller single-use scripts.
A 3rd option: we could rewrite the script to take a -e2e CLI param that changes it.

dan-f · 2017-10-18T14:44:27Z

Well, in any case, we're going to need some CLI interface by which we can specify the particular NODE_ENV. I can understand the arguments for option 1, so let's do that!

dan-f · 2017-10-18T14:57:21Z

@mattkrick - build passes w/ fixes addressing your final feedback. I'm not quite sure how to verify the prod build, so I'd appreciate your help with that!

# Conflicts: # yarn.lock

mattkrick reviewed Oct 12, 2017

View reviewed changes

dan-f force-pushed the epic7-e2e-tests branch from f4eccb0 to cff772b Compare October 12, 2017 15:15

dan-f force-pushed the epic7-e2e-tests branch from cff772b to 2c244a8 Compare October 13, 2017 15:11

dan-f commented Oct 13, 2017

View reviewed changes

jordanh reviewed Oct 13, 2017

View reviewed changes

dan-f added 5 commits October 17, 2017 10:13

WIP - e2e tests

43534c9

Webdriver and server working!

Implement first e2e test!

002650d

Implement authentication e2e tests.

9341995

Address code review feedback, clean up the test runner

3435da6

Update README.md, circle.yml for parity w/ actual node version

01187b8

dan-f changed the title ~~[WIP] Epic7 e2e tests~~ Epic7 e2e tests Oct 17, 2017

dan-f force-pushed the epic7-e2e-tests branch from 34b9f69 to 2fe4ba1 Compare October 17, 2017 14:46

Add end-to-end tests to circle build job

ee0a6f4

dan-f force-pushed the epic7-e2e-tests branch from d6fbf3f to ee0a6f4 Compare October 17, 2017 20:01

mattkrick reviewed Oct 18, 2017

View reviewed changes

Address final code review feedback

77376da

dan-f and others added 3 commits October 18, 2017 18:03

Debug circle deploy

b3a2d09

Debug circle deploy pt. 2

1955410

Merge branch 'epic7' into epic7-e2e-tests

880bc05

# Conflicts: # yarn.lock

mattkrick merged commit 9b6c5e6 into epic7 Oct 21, 2017

mattkrick deleted the epic7-e2e-tests branch October 21, 2017 02:52

mattkrick mentioned this pull request Oct 24, 2017

Try moving end-to-end tests to cypress.io #1316

Closed

		"start:tunnel": "export $(grep ^ULTRAHOOK_API_KEY .env \| tr -d '\"') && ultrahook -k $ULTRAHOOK_API_KEY dev 3000",
		"dev": "if [ ! -f \"dll/vendors.json\" ] ; then npm run build:dll ; fi ; NODE_ENV=development node --trace-warnings ./src/server/server.babel.js",

Epic7 e2e tests #1413

Epic7 e2e tests #1413

Conversation

dan-f commented Oct 12, 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

codecov bot commented Oct 12, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dan-f commented Oct 13, 2017

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

dan-f commented Oct 17, 2017

mattkrick 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

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

mattkrick commented Oct 18, 2017

dan-f commented Oct 18, 2017 • edited Loading

mattkrick commented Oct 18, 2017

dan-f commented Oct 18, 2017

dan-f commented Oct 18, 2017

dan-f commented Oct 12, 2017 •

edited

Loading

codecov bot commented Oct 12, 2017 •

edited

Loading

dan-f commented Oct 18, 2017 •

edited

Loading