Skip to content
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

Merged
merged 10 commits into from Aug 3, 2020
Merged

test: refactor tests #1040

merged 10 commits into from Aug 3, 2020

Conversation

erezrokah
Copy link
Contributor

Fixes #1031

Also renamed many tests so their description matches what we're testing.
Also renamed dev.test.js to command.dev.test.js (soon to be followed in another PR for build.test.js)

@erezrokah erezrokah changed the title Test: refactor tests test: refactor tests Jul 29, 2020
@erezrokah erezrokah added the type: chore work needed to keep the product and development running smoothly label Jul 29, 2020
const test = require('ava')
const stripAnsi = require('strip-ansi')
const cliPath = require('./utils/cliPath')
const exec = require('./utils/exec')
Copy link
Contributor Author

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.

  1. Using ./utils/exec (addons tests)
  2. Using a promisified exec duplicating ./utils/exec logic (dev command tests)
  3. Using execa (build command tests).

I aligned the places I could find to use execa and removed ./utils/exec

Copy link
Contributor

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')
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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')
Copy link
Contributor Author

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 }) => {
Copy link
Contributor Author

@erezrokah erezrokah Jul 29, 2020

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.

host,
port,
close: async () => {
const pids = await pidtree(ps.pid).catch(() => [])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erezrokah erezrokah marked this pull request as ready for review July 30, 2020 16:47
@erezrokah erezrokah requested a review from ehmicky July 30, 2020 16:47
@erezrokah
Copy link
Contributor Author

@ehmicky, sorry for the site of this PR, got too deep and had to dig my way out of it by doing more refactoring 😄

tests/utils/devServer.js Outdated Show resolved Hide resolved
tests/utils/devServer.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @erezrokah! 🚀

@erezrokah erezrokah force-pushed the test/refactor_tests branch 2 times, most recently from fa9feae to 89c6325 Compare August 3, 2020 10:02
@erezrokah erezrokah merged commit 566eed7 into master Aug 3, 2020
@erezrokah erezrokah deleted the test/refactor_tests branch August 3, 2020 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test: make it easier to add tests
2 participants