test(remote): refactor to run remote tests in a single process #1760
Conversation
This refactors our remote test driver to stop spawning multiple child processes to run our servers, and instead to run the servers in the same process. - By using the same process, we can pass configuration as a plain old JavaScript object, and not have to be adjusting the `process.env`. While writing this patch, `process.env` pollution was already found to make some tests dependent on others running first. Now, we can isolate the tests by starting a server with a private config object, and the other tests are non the wiser. - By not starting up and tear down child processes for each suite of remote tests, the full set runs much faster. In my case, running the remote tests went from ~4 minutes to ~1 minute.
| @@ -194,6 +194,10 @@ module.exports = function (log, config) { | |||
| return logInvalidContext(this, 'expired flowBeginTime') | |||
| } | |||
|
|
|||
| if (! HEX.test(metadata.flowId)) { | |||
seanmonstar
Mar 23, 2017
Author
Member
Without this, Buffer.from(flowId, 'hex') exceptions were being thrown during remote tests, but not from our handlers, so hapi was just reporting it in its weird debug way. Once they were in the same process, I noticed the odd output, and put this fix in.
Without this, Buffer.from(flowId, 'hex') exceptions were being thrown during remote tests, but not from our handlers, so hapi was just reporting it in its weird debug way. Once they were in the same process, I noticed the odd output, and put this fix in.
| // for easy promise lib switching | ||
| module.exports = require('bluebird') | ||
|
|
||
| process.on('unhandledRejection', (reason, promise) => { |
seanmonstar
Mar 23, 2017
Author
Member
The route in sign.js was through exceptions about using reply more than once, and we never dealt with the error or even really knew about it other than an odd hapi debug output. This change made it so unhandled rejections were treated as uncaught exceptions. While it might mean more crashes, it also means no errors that go unnoticed.
The route in sign.js was through exceptions about using reply more than once, and we never dealt with the error or even really knew about it other than an odd hapi debug output. This change made it so unhandled rejections were treated as uncaught exceptions. While it might mean more crashes, it also means no errors that go unnoticed.
philbooth
Mar 23, 2017
Contributor
It feels like a little bit of a surprising place to put it is all I'm thinking. I get that it's quite promisey, but it's also quite processy/servery. I'm not that bothered about it tbh, just wanted to mention it.
It feels like a little bit of a surprising place to put it is all I'm thinking. I get that it's quite promisey, but it's also quite processy/servery. I'm not that bothered about it tbh, just wanted to mention it.
| @@ -81,24 +81,24 @@ module.exports = function (log, P, isA, error, signer, db, domain, devices) { | |||
| function () { | |||
| if (publicKey.algorithm === 'RS') { | |||
| if (! publicKey.n) { | |||
| return reply(error.missingRequestParameter('n')) | |||
| throw error.missingRequestParameter('n') | |||
seanmonstar
Mar 23, 2017
Author
Member
Since this was just returning, later promises in the then chain would end up calling reply also. Hapi properly only sends 1 response, but then throws an exception at using it more than once. The exception was swallowed by the unhandled rejection. Once that was fixed, these needed to change to throws so we wouldn't call reply again.
Since this was just returning, later promises in the then chain would end up calling reply also. Hapi properly only sends 1 response, but then throws an exception at using it more than once. The exception was swallowed by the unhandled rejection. Once that was fixed, these needed to change to throws so we wouldn't call reply again.
| @@ -124,7 +124,8 @@ function create(log, error, config, routes, db, translator) { | |||
| }, | |||
| load: { | |||
| sampleInterval: 1000 | |||
| } | |||
| }, | |||
| useDomains: false | |||
seanmonstar
Mar 23, 2017
Author
Member
This made it so uncaught exceptions in our handlers and in the pre and post events would be caught in node's Domain, and then hapi would just output with it's quirky server.log() mechanism. This meant we didn't know about some errors. Domains are weird, and considered a mistake in nodejs, so lets just disable it, so we never hide exceptions.
This made it so uncaught exceptions in our handlers and in the pre and post events would be caught in node's Domain, and then hapi would just output with it's quirky server.log() mechanism. This meant we didn't know about some errors. Domains are weird, and considered a mistake in nodejs, so lets just disable it, so we never hide exceptions.
philbooth
Mar 23, 2017
Contributor
I didn't even know Domain was a thing, nice one.
I didn't even know Domain was a thing, nice one.
| console.log('\x1B[36mUnblock code:', uc, '\x1B[39m') | ||
| console.log('\x1B[36mReport link:', rul, '\x1B[39m') | ||
| } | ||
| else if (TEMPLATES_WITH_NO_CODE.has(template)) { |
seanmonstar
Mar 23, 2017
Author
Member
This was just because I kept seeing scary red No verify code match text output, even though some emails are expected to not have one.
This was just because I kept seeing scary red No verify code match text output, even though some emails are expected to not have one.
| @@ -34,116 +40,33 @@ function TestServer(config, printLogs) { | |||
| this.mailbox = mailbox(config.smtp.api.host, config.smtp.api.port, this.printLogs) | |||
| } | |||
|
|
|||
| function waitLoop(testServer, url, cb) { | |||
| request( | |||
seanmonstar
Mar 23, 2017
Author
Member
Switching to a single process increased the speed by 2x, (from 4min to 2), and then interestingly, killing this wait loop and just waiting on the promise to resolve from bin/key_server got us to 1 minute!
Switching to a single process increased the speed by 2x, (from 4min to 2), and then interestingly, killing this wait loop and just waiting on the promise to resolve from bin/key_server got us to 1 minute!
|
Magnificent! r+ |
|
This is awesome 🙌🏾 r+! |
|
|
This refactors our remote test driver to stop spawning multiple
child processes to run our servers, and instead to run the servers
in the same process.
JavaScript object, and not have to be adjusting the
process.env.While writing this patch,
process.envpollution was already foundto make some tests dependent on others running first. Now, we can
isolate the tests by starting a server with a private config object,
and the other tests are non the wiser.
remote tests, the full set runs much faster. In my case, running the
remote tests went from ~4 minutes to ~1 minute.
Before:
After: