-
Notifications
You must be signed in to change notification settings - Fork 400
Show Docker logs again in functional tests #2727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Even though the tests mysteriously fixed themselves, I researched this and could see some legitimate causes for failure. It sucks to replace one line of piped bash commands with 200 lines of JS but I think it helps buy us many things:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+wc
Thanks a tonne for this. 👍
|
||
// TODO: make these configurable when we need to start | ||
// servers for multiple apps. | ||
const appInstance = 'disco'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using process.env.NODE_APP_INSTANCE
would be a pretty easy fix here I think and buy us most of that. I think it's already passed to the tests on travis... if not you could easily add it to the env vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I thought so too but passing env vars to commands in .travis.yml is not straight forward. I think it's best to deal with it later. I think we may have to create an amo
and a disco
package.json
script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, no problem.
bin/start-func-test-server.js
Outdated
} | ||
return new Promise((resolve, reject) => { | ||
childProcess.exec(cmdString, { cwd: root }, (error, stdout, stderr) => { | ||
let gotOutput = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I found gotOutput
a bit of an odd variable name to grok at first. Maybe receivedOutput
would be better?
Fixes mozilla/addons#10539