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.

Sign up for GitHub

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

Jump to bottom

ci(test): split into multiple machines #4164

Merged
merged 2 commits into from
Feb 3, 2022
Merged

ci(test): split into multiple machines #4164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94f78ee
ci(test): split tests into multiple machines
erezrokah Feb 3, 2022
93990b5
test: add --no-worker-threads
erezrokah Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 57 additions & 11 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,55 @@ on:

jobs:
build:
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ubuntu-latest, macOS-latest, windows-latest]
node-version: ['*']
steps:
# Sets an output parameter if this is a release PR
- name: Check for release
id: release-check
run: echo "::set-output name=IS_RELEASE::true"
if: "${{ startsWith(github.head_ref, 'release-') }}"
- name: Git checkout
uses: actions/checkout@v2
if: '${{!steps.release-check.outputs.IS_RELEASE}}'
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v2
with:
node-version: ${{ matrix.node-version }}
cache: 'npm'
cache-dependency-path: 'npm-shrinkwrap.json'
check-latest: true
if: '${{!steps.release-check.outputs.IS_RELEASE}}'
- name: Install core dependencies
run: npm ci --no-audit
if: '${{!steps.release-check.outputs.IS_RELEASE}}'
- name: Install site dependencies
run: npm run site:build:install
if: '${{!steps.release-check.outputs.IS_RELEASE}}'
- name: Linting
run: npm run format:ci
if: '${{!steps.release-check.outputs.IS_RELEASE}}'
- name: Run unit tests
run: npm run test:ci:ava:unit
if: '${{!steps.release-check.outputs.IS_RELEASE}}'
test:
runs-on: ${{ matrix.os }}
timeout-minutes: 30
strategy:
matrix:
os: [ubuntu-latest, macOS-latest, windows-latest]
node-version: [12.x, '*']
machine: ['0', '1', '2', '3', '4', '5', '6']
Copy link
Contributor

@ehmicky ehmicky Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good idea!

I am wondering about how this will scale. As we add more tests, we might be likely to distribute too many or not enough tests to a specific number, because that distribution is manual instead of being automated.

Not sure if this is worth exploring for this initial PR, but I am wondering whether we could hash each test filename (or file path relative to the repository root directory), then use a modulo on them to know to which machine to assign?
For example, if we had 16 machines, we could make a SHA-1 of each test filename, then use the last hexadecimal number to decide on which machine to use. We could make the base arbitrary (not only hexadecimal / base 16) so that increasing/decreasing the number of machines just works.
While this would be a more thorough initial implementation work, this would decrease the amount of maintenance: we would not need to figure out which digit to assign to each test file, instead the only thing we'd need to remember would be to keep test files small. Also, we would be able to optimize the best amount of machines easily by just trying different ones and see how long the CI tests take.

What do you think?

Note: that's just an idea. The initial PR looks ready to ship otherwise as it already helps a lot with the performance as is.
Also, we should probably merge this asap to avoid merge conflicts.

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 is a good idea, and I think we should definitely find a way to automate the distribution.
I like the idea of small specs (maybe we can enforce via a lint rule), making the need to load balance less important.


exclude:
- os: macOS-latest
node-version: '12.x'
- os: windows-latest
node-version: '12.x'
fail-fast: false

steps:
# Sets an output parameter if this is a release PR
- name: Check for release
Expand All @@ -32,36 +68,35 @@ jobs:
run: |
REG ADD HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\TCPIP\Parameters /v MaxUserPort /t REG_DWORD /d 32768 /f
REG ADD HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\TCPIP\Parameters /v TcpTimedWaitDelay /t REG_DWORD /d 30 /f
if: "${{ matrix.os == 'windows-latest' }}"
if: "${{ matrix.os == 'windows-latest' && !steps.release-check.outputs.IS_RELEASE }}"
- name: Git checkout
uses: actions/checkout@v2
with:
fetch-depth: 0
if: '${{!steps.release-check.outputs.IS_RELEASE}}'
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v2
with:
node-version: ${{ matrix.node-version }}
cache: 'npm'
cache-dependency-path: 'npm-shrinkwrap.json'
check-latest: true
if: '${{!steps.release-check.outputs.IS_RELEASE}}'
- name: Install core dependencies
run: npm ci --no-audit
- name: Install site dependencies
run: npm run site:build:install
- name: Linting
run: npm run format:ci
if: "${{ matrix.node-version == '*' && !steps.release-check.outputs.IS_RELEASE}}"
if: '${{!steps.release-check.outputs.IS_RELEASE}}'
- name: Determine Test Command
uses: haya14busa/action-cond@v1
id: testCommand
with:
cond: ${{ github.event_name == 'pull_request' }}
if_true: 'npm run test:affected ${{ github.event.pull_request.base.sha }}' # on pull requests test with the project graph only the affected tests
if_false: 'npm run test:ci' # on the base branch run all the tests as security measure
if_false: 'npm run test:ci:ava:integration' # on the base branch run all the tests as security measure
if: '${{ !steps.release-check.outputs.IS_RELEASE }}'
- name: Prepare tests
run: npm run test:init
- name: Tests
if: '${{ !steps.release-check.outputs.IS_RELEASE }}'
- name: Tests
run: ${{ steps.testCommand.outputs.value }}
env:
# GitHub secrets are not available when running on PR from forks
Expand All @@ -74,18 +109,29 @@ jobs:
# Changes the polling interval used by the file watcher
CHOKIDAR_INTERVAL: 20
CHOKIDAR_USEPOLLING: 1
- name: Get test coverage flags

# split tests across multiple machines
CI_NODE_INDEX: ${{ matrix.machine }}
CI_NODE_TOTAL: 7
if: '${{ !steps.release-check.outputs.IS_RELEASE }}'
- name: Get test coverage flags
id: test-coverage-flags
run: |-
os=${{ matrix.os }}
node=${{ matrix.node-version }}
echo "::set-output name=os::${os/-latest/}"
echo "::set-output name=node::node_${node//[.*]/}"
shell: bash
- uses: codecov/codecov-action@v2
if: '${{ !steps.release-check.outputs.IS_RELEASE }}'
- uses: codecov/codecov-action@v2
continue-on-error: true
with:
file: coverage/coverage-final.json
flags: ${{ steps.test-coverage-flags.outputs.os }},${{ steps.test-coverage-flags.outputs.node }}
if: '${{ !steps.release-check.outputs.IS_RELEASE }}'
all:
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 is here strictly to make branch protection rules easier, to avoid changing those based on matrix naming

needs: [build, test]
runs-on: ubuntu-latest
steps:
- name: Log success
run: echo "Finished running all tests"
6 changes: 3 additions & 3 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ site/src/**/*.md

# tests
.eslintcache
tests/hugo-site/resources
tests/hugo-site/out
tests/hugo-site/.hugo_build.lock
tests/integration/hugo-site/resources
tests/integration/hugo-site/out
tests/integration/hugo-site/.hugo_build.lock
12 changes: 5 additions & 7 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@
"format:check:prettier": "cross-env-shell prettier --check $npm_package_config_prettier",
"format:fix:prettier": "cross-env-shell prettier --write $npm_package_config_prettier",
"test:dev": "run-s test:init:* test:dev:*",
"test:ci": "run-s test:ci:*",
"test:init": "run-s test:init:*",
"test:init:cli-version": "npm run start -- --version",
"test:init:cli-help": "npm run start -- --help",
"test:init:eleventy-deps": "npm ci --prefix tests/eleventy-site --no-audit",
"test:init:hugo-deps": "npm ci --prefix tests/hugo-site --no-audit",
"test:init:eleventy-deps": "npm ci --prefix tests/integration/eleventy-site --no-audit",
"test:init:hugo-deps": "npm ci --prefix tests/integration/hugo-site --no-audit",
"test:dev:ava": "ava --verbose",
"test:ci:ava": "c8 -r json ava",
"test:ci:ava:unit": "c8 -r json ava --no-worker-threads tests/unit/**/*.test.js tools/**/*.test.js",
"test:ci:ava:integration": "c8 -r json ava --concurrency 1 --no-worker-threads tests/integration/**/*.test.js",
"test:affected": "node ./tools/affected-test.js",
"e2e": "node ./tools/e2e/run.mjs",
"docs": "node ./site/scripts/docs.js",
Expand Down Expand Up @@ -215,10 +215,8 @@
},
"ava": {
"files": [
"site/**/*.test.js",
"src/**/*.test.js",
"tools/**/*.test.js",
"tests/*.test.js"
"tests/**/*.test.js"
],
"cache": true,
"concurrency": 5,
Expand Down
8 changes: 8 additions & 0 deletions src/utils/rules-proxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,21 @@ const { fileExistsAsync } = require('../lib/fs')
const { NETLIFYDEVLOG } = require('./command-helpers')
const { parseRedirects } = require('./redirects')

const watchers = []

const onChanges = function (files, listener) {
files.forEach((file) => {
const watcher = chokidar.watch(file)
watcher.on('change', listener)
watcher.on('unlink', listener)
watchers.push(watcher)
})
}

const getWatchers = function () {
return watchers
}

const getLanguage = function (headers) {
if (headers['accept-language']) {
return headers['accept-language'].split(',')[0].slice(0, 2)
Expand Down Expand Up @@ -97,4 +104,5 @@ module.exports = {
onChanges,
getLanguage,
createRewriter,
getWatchers,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we can close the watchers in the tests

}
Loading