Add End-to-End Testing Utility #778

nsakaimbo · 2020-02-19T22:22:25Z

Description

This PR adds tooling to enable easier invocation of the end-to-end suite locally, and sets the stage for cross-platform invocation in the near future.

To-date, running tests locally has been a clumsy and cumbersome process in which developers had to cross-reference the existing Circle CI script to determine the steps required to invoke the test suite. This involved multiple manual steps:

Setting up the necessary environment variables (test credentials)
initializing the application
Initializing chromedriver
Running the tests via mocha, and finally
Shutting down and re-starting chromedriver and the application between test runs

Debugging and augmenting the test suite locally has been significantly hampered by the lack of tooling to enable faster and more efficient iteration. The goal of this utility is to significantly reduce this overhead such that invoking the tests only requires two steps:

Setting up the necessary environment variables (which are now made explicit), and
invoking the tests with make e2e or npm run e2e

At the end of teach test run (or when the suite errors or is interrupted via CTRL+C), all processes are automatically shut down. Automated process cleanup allows repeated tests runs with little to no manual intervention between each test run.

To Test

Set required environment variables (username, password, email)
Invoke tests with make e2e or npm run e2e

Alternatives

Alternatives using make and bash were explored, but ultimately did not function as desired. bash in particular seems promising and might be worth re-visiting down the road.

This reverts commit ab06fe7.

dmsnell · 2020-02-19T22:25:32Z

test/run/checks.js

@@ -0,0 +1,28 @@
+
+function usernameExists() {
+    return ( process.env.E2EUSERNAME && process.env.E2EUSERNAME !== '' ) ? true : false;


fun fact: this is equivalent to Boolean( process.env.E2EUSERNAME && … ) and also to !! (process.env.E2EU…)

just noting since the ternary is there a bit surprisingly - was there something odd about the condition that prevented us from casting it to a boolean?

anyway, not a big deal

... was there something odd about the condition that prevented us from casting it to a boolean?

Nope - just my lack of experience writing idiomatic JavaScript. 😆Thanks for the suggestion - however casting to Boolean is now unnecessary as I went ahead and used the suggested credentials check refactor in your comment below, which works pretty well.

dmsnell · 2020-02-19T22:35:26Z

test/run/run-e2e-tests.js

+    try {
+        checkCredentials();
+
+        const timestamp = ( new Date() ).toJSON().replace( /:/g, '-' );


can we change this to be some explicit format? I find it near impossible to know the intention behind it, especially if for some reason the JSON serialization changes and we end up with formats that we don't expect.

const formatYMD = instant => { const year = instant.getFullYear(); const month = `${ instant.getMonth() + 1 }`.padStart(2, '0'); const day = `${ instant.getDate() }`.padStart(2, '0'); return `${ year }-${ month }-${ date }`; } … const timestamp = formatYMD( new Date() );

inline isn't so bad: it's still explicit but it is uglier on account of needing to cast numbers to strings

const now = new Date(); const timestamp = `${now.getFullYear()}-${`${now.getMonth() + 1}`.padStart(2,'0')}-${`${now.getDate()}`.padStart(2, '0')}`

Agree - this snippet of code isn't intuitive at first glance. However in this specific case, all the snippet is doing is some very minor re-formatting and a comment to clarify is sufficient (add here):

// Replace `:` with `-` to format timestamp as YYYY-MM-DDTHH-MM-SS.mmmZ

I think a comment here is perfectly fine and creating an entire method to replicate all desired format components would add unnecessary complexity (and verbosity) to the file. If we choose to have a more customized date format in the future, we can create a separate method at that point.

the comment is helpful enough since it shows what you wanted. my point was made though since I didn't expect .mmmZ 😄

dmsnell · 2020-02-19T22:38:10Z

test/run/init_app.js

+const { BUILT_APP_DIR } = require( './paths' );
+
+function initApp( log ) {
+    const startCmd = './'


was this more significant before? what's the purpose of splitting it from ./WordPress.com?

This was in anticipation of Windows-specific syntax, however I've been able to determine that will be unnecessary and we can stick with ./WordPress.com, which is nice.

dmsnell · 2020-02-19T22:46:28Z

test/run/init_app.js

+    } );
+
+    return app;
+}


Just playing around here…

function run( cwd, command, args, output ) { const app = spawn( command, args, { stdio: [ 'ignore', output, output ], detached: true, cwd } ); app.on( 'error' => { throw new Error( `failed to initialize ${command}` ); } ); return app; } const app = run( BUILT_APP_DIR, './WordPress.com', [ '--disable-renderer-backgrounding', '--disable-http-cache', '--start-maximized', '--remote-debugging-port=9222', ], appLog ); await delay(5000); const driver = run( PROJECT_DIR, 'npx', [ 'chromedriver', '--port=9515', '--verbose', ], driverLog );

There's a part of me feeling like we could make a simple abstraction over running the process and then eliminate these two init… files. I'm nervous about having the straightforward logic of starting the tests be obscured by being hidden in a number of split up files.

Even the appLog and driverLog, for example, could be inlined in run-e2e-tests.js

For transparency, my anxiety about this is from the fact that the test runner systems are always one-off projects and have their own language and style. In these cases I definitely value having more stuff in one file where I can see it in my editor all at once over separating the pieces into separate files and modules.

While it may seem to disagree with my play above to introduce the process abstraction I'm still drawing a line between "this is a thing that spawns a process" and "start the app somehow" and "start the driver somehow." Whereas having the startup commands and CLI args in the test runner itself the mechanisms going on are obvious even if there's something hidden in the process abstraction (redirection of stdout and stderr). The pattern should be obvious enough from the cmd, [ arg1, arg2 ] syntax

There's a part of me feeling like we could make a simple abstraction over running the process and then eliminate these two init… files

Agreed - having the various components initialized in separate files creates unnecessary indirection and overhead when navigating the script. I refactored to inline everything, and the proposed refactor for the driver and app initialization is a great idea. 👍 See refactor here.

dmsnell · 2020-02-19T22:52:56Z

test/run/run-e2e-tests.js

+
+async function run() {
+    try {
+        checkCredentials();


I'l make a similar comment here about my own anxiety and obscuring what's happening vs. hiding how it's happening. this doesn't give me much information about the credentials even though I assume it's checking something.

Another formulation might abstract the check and expose the things being checked…

async function run() { const requiredENVs = [ 'E2EUSERNAME', 'E2EPASSWORD', 'E2E_MAILOSAUR_INBOX' ]; const missingENVs = requiredENVs.filter( name => ! process.env[name] || process.env[name] === '' ); if ( missingENVs.length ) { throw new Error( `Missing non-empty ENV for: ${ missingENVs.join(', ')` ); } … }

Agreed - see my response to your prior comment. Everything's been refactored inline. Also, I like the above as a much more concise way to check the required environment variables. See refactor here.

dmsnell · 2020-02-19T22:54:57Z

test/run/run-e2e-tests.js

+    finally {
+        process.exit();
+    }
+}


just because the entire function body is a try maybe it's worth splitting? probably fine as-is, but feels a little silly having a big function body inside a try

async function run_unsafe() { … } async function run() { try { run_unsafe(); } catch (err) { console.error( err ); } finally { process.exit() } }

In this specific case, the entirety of the logic directly encapsulated within the try block is evident at a glance, and further encapsulation in a wrapper method is unnecessary (and IMO adds an unnecessary level of indirection, and could be even be confusing). My preference here is to leave it as-is.

dmsnell

Thanks for all the work you've put into this. I know from our conversations that you've put in far more on this than it may look just from the produced code.

Feel free to add your thoughts and disagree, but when I read this I look at it like systems code/run scripts and I feel different pressures than I would with normal application code. I care much more about knowing what is going on than I do about what should be going on, if that makes any sense. I want to see as little modularization and abstraction as is reasonable, compared to when I do want to abstract away details from normal code we write.

Put another way, I find more value in showing in a large function with separate "phases" or sections the actual CLI command and args than I do in delegating that work to separate functions, or even more so, to separate modules.

function run() {
  // check required ENV vars
  …

  // start the Electron app
  …

  // start Chromedriver so it hooks into the app
  …

  // run the actual tests
  …
}

there's extra noise here due to the node/shell mismatch but I feel like we can abstract the concept of what we're doing (as suggesting in another comment I left) without hiding the specifics of what we're doing.

dmsnell · 2020-02-19T23:04:46Z

.circleci/config.yml

-                  ./node_modules/.bin/chromedriver --port=9515 --verbose > "$DRIVERLOG" 2>&1 &
-                  ./node_modules/.bin/mocha test/tests/e2e.js
-                  osascript -e 'quit app "WordPress.com"'
+                  make e2e


should we just call npm run e2e here instead of calling through make?

I think it might be nice to have the option of calling the tests either way. There's also some benefit to exposing/documenting critical workflow tooling in the makefile. In addition, all paths in the test script are absolute, which, combined with make, gives the developer flexibility in how they are invoked. (e.g. tweaking something from within the Calypso submodule and invoking with make -f ./Makefile e2e (tested and this works, btw)).

dmsnell · 2020-02-19T23:16:51Z

test/run/init_tests.js

+        } );
+        tests.stdout.pipe( process.stdout );
+        tests.stderr.pipe( process.stderr );
+    } );


given my comment about the process runner, this one almost seems like it would be fine inline in the main test runner module. it's ugly, but if we remove some of the variables here it's not so bad, especially since we remove a level of Promise

async function run() { … // run the tests const testPath = path.join( __dirname, '../../test/tests/e2e.js' ); // by default should pipe stdout and stderr to this parent process // will throw an error on exception execSync( `npx mocha ${ testPath } --timeout 20000` ); }

Great suggestion. Refactored inline with execSync here. We have to add the additional stdio: 'inherit' option so we can leverage streamed (and not buffered) output to the current shell, and it works great. 👍

dmsnell · 2020-02-21T20:18:04Z

test/run/run-e2e-tests.js

+function initLogs( timestamp ) {
+    const dir = path.join( PROJECT_DIR, 'test', 'logs', `${ timestamp }` );
+
+    if ( !existsSync( dir ) ) {


note that this introduces a race condition that may not protect us. the directory could be created between the call to exists and the call to mkdir

probably simpler to just call mkdirSync and handle failure if the directory already exists. it may only fail in windows.

in other words, for the same level of safety we can skip existsSync()

try { mkdirSync( dir, { recursive: true } ); } catch (e) { // pass }

or do the more elegant/complete thing

try { mkdirSync( dir, { recursive: true } ); } catch (e) { if (!existsSync(dir)) { console.error('Could not create log directory'); process.exit(1); } }

According to the docs it may never throw an error if recursive: true and we're not creating / on Windows

Calling fs.mkdir() when path is a directory that exists results in an error only when recursive is false.

so in that case we may have the same safety as with existsSync() without any further try/catch. if it fails the build crashes as it must.

… mkdirSync(dir, {recursive: true }); …

Thanks for double-checking the usage here. Simplified this in f4021b9

dmsnell · 2020-02-21T20:22:32Z

test/run/run-e2e-tests.js

+        console.error( err );
+    }
+    finally {
+        process.exit();


it's probably worth a comment here explaining that this kills the child processes which otherwise might continue to run. looking at the code it's odd seeing finally { process.exit() } as the last real line of execution. someone might come around and "improve" the code by removing it

... someone might come around and "improve" the code by removing it

😆Good point. Added comment in 87b55d5.

dmsnell

I did not test this but from a code-audit perspective it looks quite good. Thanks for staying diligent on it and for being flexible to explore a few different approaches.

nsakaimbo added 10 commits February 12, 2020 15:38

[tests] Add e2e test utility

2333574

Use npx to init .bin modules

ccbbba3

[Makefile] Do it all with make!

ab06fe7

Revert "[Makefile] Do it all with make!"

16164c6

This reverts commit ab06fe7.

Rename E2E const to E2E_TESTS

4a1239c

Move to test/run folder

6a4aa12

Add E2E_MAILOSAUR_INBOX env var

7f43746

Add instructions to README

db00e45

minor fomatting fix

25d00d4

Refactor credentials check

a82d77a

nsakaimbo requested review from Stojdza and a team February 19, 2020 22:22

dmsnell reviewed Feb 19, 2020

View reviewed changes

nsakaimbo added 4 commits February 20, 2020 20:41

Use inline credentials check

eb8ecbe

Add comment to clarify date formatting

87555c8

Inline all functional components

c1862f5

Invoke tests in main fn body with execSync

ed6b772

nsakaimbo force-pushed the add-e2e-test-runner branch from 1b5cc95 to ed6b772 Compare February 21, 2020 15:01

dmsnell reviewed Feb 21, 2020

View reviewed changes

nsakaimbo added 2 commits February 21, 2020 16:20

Remove unnecessary existsSync check

f4021b9

Add comment regarding call to process.exit

87b55d5

minor re-formatting

19a19ef

dmsnell approved these changes Feb 22, 2020

View reviewed changes

nsakaimbo merged commit 273c3b3 into develop Feb 24, 2020

nsakaimbo deleted the add-e2e-test-runner branch February 24, 2020 15:51

nsakaimbo mentioned this pull request Dec 13, 2021

Implement End-to-End Tests on Windows Automattic/wp-calypso#59136

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add End-to-End Testing Utility #778

Add End-to-End Testing Utility #778

nsakaimbo commented Feb 19, 2020

dmsnell Feb 19, 2020

nsakaimbo Feb 21, 2020

dmsnell Feb 19, 2020

nsakaimbo Feb 21, 2020

dmsnell Feb 21, 2020

dmsnell Feb 19, 2020

nsakaimbo Feb 21, 2020

dmsnell Feb 19, 2020

nsakaimbo Feb 21, 2020

dmsnell Feb 19, 2020 •

edited

Loading

nsakaimbo Feb 21, 2020

dmsnell Feb 19, 2020

nsakaimbo Feb 21, 2020

dmsnell left a comment

dmsnell Feb 19, 2020

nsakaimbo Feb 21, 2020 •

edited

Loading

dmsnell Feb 19, 2020

nsakaimbo Feb 21, 2020 •

edited

Loading

dmsnell Feb 21, 2020

dmsnell Feb 21, 2020

nsakaimbo Feb 21, 2020

dmsnell Feb 21, 2020

nsakaimbo Feb 21, 2020

dmsnell left a comment

		@@ -0,0 +1,28 @@

		function usernameExists() {
		return ( process.env.E2EUSERNAME && process.env.E2EUSERNAME !== '' ) ? true : false;

Add End-to-End Testing Utility #778

Add End-to-End Testing Utility #778

Conversation

nsakaimbo commented Feb 19, 2020

Description

To Test

Alternatives

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

dmsnell Feb 19, 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

dmsnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsakaimbo Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsakaimbo Feb 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell left a comment

Choose a reason for hiding this comment

dmsnell Feb 19, 2020 •

edited

Loading

nsakaimbo Feb 21, 2020 •

edited

Loading

nsakaimbo Feb 21, 2020 •

edited

Loading