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

Fix for flaky Windows tests preventing deployment #2647

Merged
merged 5 commits into from
Mar 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
107 changes: 84 additions & 23 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,44 +16,105 @@ on:
type: boolean

workflow_dispatch:
inputs:
runner:
description: Run tests on
type: choice
default: ubuntu-latest
options:
- macos-latest
- ubuntu-latest
- windows-latest

jobs:
build:
name: Build & Test
strategy:
matrix:
os: [ubuntu-latest, windows-latest]
include:
- os: ubuntu-latest
upload-artifact: ${{ inputs.upload-artifact }}
runs-on: ${{ matrix.os }}
name: Build
runs-on: ${{ github.event.inputs.runner || 'ubuntu-latest' }}

env:
ENVIRONMENT: production

steps:
- uses: actions/checkout@v3
- name: Checkout code
uses: actions/checkout@v3

- name: Setup node
- name: Restore build
uses: actions/cache@v3
id: build-cache

with:
key: build-cache-${{ runner.os }}-${{ github.sha }}
path: deploy/public

- name: Setup Node.js
uses: actions/setup-node@v3
if: steps.build-cache.outputs.cache-hit != 'true'

with:
node-version-file: '.nvmrc'
cache: 'npm'
cache: npm
node-version-file: .nvmrc

- name: Install dependencies
if: steps.build-cache.outputs.cache-hit != 'true'
run: npm ci

- name: Build, lint and test
run: npm test -- --color --maxWorkers=2
- name: Build
if: steps.build-cache.outputs.cache-hit != 'true'
run: npm run build

- name: Check broken links
run: npm run check-links

# Share data between the build and deploy jobs so we don't need to run `npm run build` again on deploy
# Upload the deploy folder as an artifact so it can be downloaded and used in the deploy job
# Share data between the build and deploy jobs so we don't need to run `npm run build` again on deploy
# Upload the deploy folder as an artifact so it can be downloaded and used in the deploy job
- name: Upload artifact
uses: actions/upload-artifact@v3
if: ${{ matrix.upload-artifact }}
if: ${{ inputs.upload-artifact }}
with:
name: build
path: deploy/**
retention-days: 1
name: build
path: deploy/**
retention-days: 1

test:
name: ${{ matrix.task.description }}
runs-on: ${{ github.event.inputs.runner || 'ubuntu-latest' }}
needs: [build]

strategy:
fail-fast: false

matrix:
task:
- description: Lint Sass
run: npm run lint:scss

- description: Lint JavaScript
run: npm run lint:js

- description: Lint HTML
run: npm run lint:html --ignore-scripts

- description: JavaScript tests
run: npx jest --color --maxWorkers=2

- description: Check broken links
run: npm run check-links

steps:
- name: Checkout
uses: actions/checkout@v3

- name: Setup Node.js
uses: actions/setup-node@v3
with:
cache: npm
node-version-file: .nvmrc

- name: Install dependencies
run: npm ci

- name: Restore build
uses: actions/cache/restore@v3
with:
key: build-cache-${{ runner.os }}-${{ github.sha }}
path: deploy/public

- name: Run task
run: ${{ matrix.task.run }}
7 changes: 2 additions & 5 deletions jest-puppeteer.config.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
const configPaths = require('./lib/paths.js')

const PORT = configPaths.testPort

module.exports = {
browserContext: 'incognito',
server: {
command: 'node tasks/test-serve.js',
launchTimeout: 30000,
port: PORT
command: 'npm run serve',
port: configPaths.testPort
}
}
6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@
"scripts": {
"postinstall": "npm ls --depth=0",
"build": "node tasks/build.js",
"start": "node tasks/serve.js",
"start": "node tasks/start.js",
"serve": "node tasks/serve.js",
"pretest": "npm run build",
"test": "npm run lint && jest",
"lint": "npm run lint:js && npm run lint:scss && npm run lint:html",
"lint": "npm run lint:js && npm run lint:scss && npm run lint:html --ignore-scripts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When running the "All in one" task npm test we have already built the site using pretest

So I've added --ignore-scripts to skip prelint:html to avoid a double build

Copy link
Member

@romaricpascal romaricpascal Feb 28, 2023

Choose a reason for hiding this comment

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

question If we're doing this to avoid the double build when running test, because both pretest and prelint:html run a build, would this addition be better suited to the test command? Can it run npm run lint -- --ignore-scripts to append ignore-scripts to the lint command (and as such to its last lint:html command)? No worries if it can't though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it work? Happy to switch it

Only worry is "lint" being reordered in future and the flag goes to the wrong script

Copy link
Member

Choose a reason for hiding this comment

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

Seems to work, yup (🤯), but good shout on the reordering. Feels odd that, when run through lint, lint:html won't build, but when run on its own it will. If it becomes an issue we can always adjust then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah maybe aligning it with govuk-frontend was the wrong move?

Where only "test" runs a build via "pretest"

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand your last answer, sorry 😔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry. I mean the linting tasks on govuk-frontend don't run a build, so I copied that here

Might be worth trying to tighten up what runs when

"prelint:html": "npm run build",
"lint:html": "html-validate \"deploy/public/**/index.html\"",
"lint:js": "eslint --cache --cache-location .cache/eslint --cache-strategy content --color --ignore-path .gitignore \"**/*.{cjs,js,mjs}\"",
Expand Down
27 changes: 9 additions & 18 deletions tasks/serve.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,14 @@
const path = require('path')
const { createServer } = require('http')

const browsersync = require('../lib/metalsmith-browser-sync') // setup synchronised browser testing
const metalsmith = require('../lib/metalsmith') // configured static site generator
const connect = require('connect')
const serveStatic = require('serve-static')

const paths = require('../lib/paths.js') // specify paths to main working directories
const paths = require('../lib/paths.js')

// setup synchronised browser testing
metalsmith.use(browsersync({
ghostMode: false, // Ghost mode tries to check the same input across examples.
open: false, // When making changes to the server, we don't want multiple windows opening.
server: paths.public, // server directory
files: [
path.join(paths.source, '**/*'),
path.join(paths.views, '**/*'),
path.normalize('node_modules/govuk-frontend/**/*')
] // files to watch
}))
// Create a simple server for serving static files
const app = connect().use(serveStatic(paths.public))
const server = createServer(app)

// build to destination directory
metalsmith.build(function (err, files) {
if (err) { throw err }
server.listen(paths.testPort, () => {
console.log(`Server started at http://localhost:${paths.testPort}`)
})
23 changes: 23 additions & 0 deletions tasks/start.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const path = require('path')

const browsersync = require('../lib/metalsmith-browser-sync') // setup synchronised browser testing
const metalsmith = require('../lib/metalsmith') // configured static site generator

const paths = require('../lib/paths.js') // specify paths to main working directories

// setup synchronised browser testing
metalsmith.use(browsersync({
ghostMode: false, // Ghost mode tries to check the same input across examples.
open: false, // When making changes to the server, we don't want multiple windows opening.
server: paths.public, // server directory
files: [
path.join(paths.source, '**/*'),
path.join(paths.views, '**/*'),
path.normalize('node_modules/govuk-frontend/**/*')
] // files to watch
}))

// build to destination directory
metalsmith.build(function (err, files) {
if (err) { throw err }
})
30 changes: 0 additions & 30 deletions tasks/test-serve.js

This file was deleted.