-
Notifications
You must be signed in to change notification settings - Fork 373
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
test: refactor tests #1040
test: refactor tests #1040
Conversation
const test = require('ava') | ||
const stripAnsi = require('strip-ansi') | ||
const cliPath = require('./utils/cliPath') | ||
const exec = require('./utils/exec') |
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.
./utils/exec
is a promisified version of node exec
.
We were actually spawning commands in 3 different ways in the tests.
- Using
./utils/exec
(addons tests) - Using a promisified
exec
duplicating./utils/exec
logic (dev command tests) - Using
execa
(build command tests).
I aligned the places I could find to use execa
and removed ./utils/exec
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.
YES! 🎉
Child process spawning is complex :P
@@ -0,0 +1,793 @@ | |||
const test = require('ava') |
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.
This file was refactored and renamed from dev.test.js
.
Each test creates a dedicated site with the relevant settings just for that test
tests/command.dev.test.js
Outdated
const response1 = await fetch(`${server.url}/not-foo`).then(r => r.text()) | ||
const response2 = await fetch(`${server.url}/not-foo/`).then(r => r.text()) | ||
|
||
// TODO: check why this doesn't redirect |
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.
Kept this from the original test, still not sure what I'm missing here
@@ -0,0 +1,29 @@ | |||
const test = require('ava') |
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.
This test was inside dev.test.js
.
Since it tests a different command I moved it to a separate file.
const tempy = require('tempy') | ||
const os = require('os') | ||
|
||
const createSiteBuilder = ({ siteName }) => { |
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.
At some point I would like to add withUISetting
which will use the API to create and update the site.
8faed26
to
2da6794
Compare
host, | ||
port, | ||
close: async () => { | ||
const pids = await pidtree(ps.pid).catch(() => []) |
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.
@ehmicky, sorry for the site of this PR, got too deep and had to dig my way out of it by doing more refactoring 😄 |
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.
Thanks @erezrokah! 🚀
fa9feae
to
89c6325
Compare
89c6325
to
b85a7d4
Compare
b85a7d4
to
0f0d6bd
Compare
Fixes #1031
Also renamed many tests so their description matches what we're testing.
Also renamed
dev.test.js
tocommand.dev.test.js
(soon to be followed in another PR forbuild.test.js
)