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

Bump Node to v18.18.1; Remove puppeteer #7274

Merged
merged 23 commits into from
Oct 13, 2023
Merged

Bump Node to v18.18.1; Remove puppeteer #7274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9a67186
[CHORE] Bumping Docker to use node-18.18.0-slim base image.
1Copenut Oct 10, 2023
908fbd9
Added clarifying comments to Dockerfile.
1Copenut Oct 11, 2023
d606b47
Revert. Purposely failing spec to verify screenshot.
1Copenut Oct 11, 2023
b89e5fe
Revert purposely failing spec to verify screenshot.
1Copenut Oct 11, 2023
7514cd8
Removing Puppeteer dependencies and script calls.
1Copenut Oct 11, 2023
4252d88
Deleting Puppeteer script for running a11y checks on docs pages.
1Copenut Oct 11, 2023
3820f9b
Updating automated a11y testing docs.
1Copenut Oct 11, 2023
1012b27
Bumping nvmrc to 18.18.0 for daily Node usage.
1Copenut Oct 11, 2023
06b46b0
Removed last Puppeteer script call and dev dependency.
1Copenut Oct 12, 2023
5f8b874
Bumping Node image. Added node user to audio, video groups.
1Copenut Oct 12, 2023
6d01521
Upgrading scripts to use Docker container v5.5.
1Copenut Oct 12, 2023
9e04999
Moving to Node v18.18.1 for daily use. Restored webpack-dev-server de…
1Copenut Oct 12, 2023
509b13e
Added pinned ws type to remove linting error.
1Copenut Oct 12, 2023
fd1dea9
Added option to record video on Cypress spec failure.
1Copenut Oct 12, 2023
b532bea
[REVERT] Purposely failing Cypress spec to test video artifact.
1Copenut Oct 12, 2023
66526b8
Forgot to set video record to true.
1Copenut Oct 12, 2023
4edbee5
Removing after:spec logic not necessary until Cypress 13 migration.
1Copenut Oct 12, 2023
01c7bce
Reverting purposely failing test and video output after confirmation.
1Copenut Oct 12, 2023
72cd3cb
Final dependency version cleanup.
1Copenut Oct 12, 2023
1bcd40f
Merge branch 'main' into chore/docker-node-18.18.0
1Copenut Oct 13, 2023
e24cb34
Remove unnecessary `@types/ws` dependency/pin by upgrading `@types/no…
cee-chen Oct 13, 2023
9d5cb93
Fix type error from upgrading `@types/node`
cee-chen Oct 13, 2023
04e4d16
Remove `@types/node` entirely and let npm figure out the types it needs
cee-chen Oct 13, 2023
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
3 changes: 3 additions & 0 deletions .buildkite/pipelines/pipeline_pull_request_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ steps:
if: build.branch != "main"
artifact_paths:
- "cypress/screenshots/**/*.png"
- "cypress/videos/**/*.mp4"

- command: .buildkite/scripts/pipeline_test.sh
label: ":cypress: Cypress tests on React 17"
Expand All @@ -60,6 +61,7 @@ steps:
if: build.branch != "main"
artifact_paths:
- "cypress/screenshots/**/*.png"
- "cypress/videos/**/*.mp4"

- command: .buildkite/scripts/pipeline_test.sh
label: ":cypress: Cypress tests on React 18"
Expand All @@ -70,3 +72,4 @@ steps:
if: build.branch != "main"
artifact_paths:
- "cypress/screenshots/**/*.png"
- "cypress/videos/**/*.mp4"
8 changes: 4 additions & 4 deletions .buildkite/scripts/pipeline_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ DOCKER_OPTIONS=(
--user="$(id -u):$(id -g)"
--volume="$(pwd):/app"
--workdir=/app
docker.elastic.co/eui/ci:5.3
docker.elastic.co/eui/ci:5.5
)

case $TEST_TYPE in
Expand Down Expand Up @@ -41,17 +41,17 @@ case $TEST_TYPE in

cypress:16)
echo "[TASK]: Running Cypress tests against React 16"
DOCKER_OPTIONS+=(bash -c "/opt/yarn*/bin/yarn && yarn test-cypress --node-options=--max_old_space_size=2048 --react-version=16")
1Copenut marked this conversation as resolved.
Show resolved Hide resolved
DOCKER_OPTIONS+=(bash -c "/opt/yarn*/bin/yarn && yarn cypress install && yarn test-cypress --node-options=--max_old_space_size=2048 --react-version=16")
;;

cypress:17)
echo "[TASK]: Running Cypress tests against React 17"
DOCKER_OPTIONS+=(bash -c "/opt/yarn*/bin/yarn && yarn test-cypress --node-options=--max_old_space_size=2048 --react-version=17")
DOCKER_OPTIONS+=(bash -c "/opt/yarn*/bin/yarn && yarn cypress install && yarn test-cypress --node-options=--max_old_space_size=2048 --react-version=17")
;;

cypress:18)
echo "[TASK]: Running Cypress tests against React 18"
DOCKER_OPTIONS+=(bash -c "/opt/yarn*/bin/yarn && yarn test-cypress --node-options=--max_old_space_size=2048")
DOCKER_OPTIONS+=(bash -c "/opt/yarn*/bin/yarn && yarn cypress install && yarn test-cypress --node-options=--max_old_space_size=2048")
;;

*)
Expand Down
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
18.17.1
18.18.1
8 changes: 1 addition & 7 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"main": "lib",
"module": "es",
"types": "eui.d.ts",
"docker_image": "18.17.1",
"docker_image": "18.18.1",
"engines": {
"node": "16.x || 18.x || >=20.0"
},
Expand All @@ -32,17 +32,14 @@
"test": "yarn lint && yarn test-unit",
"test-ci": "yarn test && yarn test-cypress",
"test-unit": "node ./scripts/test-unit",
"test-a11y": "node ./scripts/a11y-testing",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the command to call our older Puppeteer tests to run a11y checks against the docs pages. I haven't removed the script because it'll be repurposed with Cypress at some point in the future.

Copy link
Member

@cee-chen cee-chen Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would strongly rather see us rip it all out now (the script file, the start-test-server-and-a11y-test script, and the @axe-core/puppeteer+puppeteer dependencies.

There's honestly very little chance we'll end up using Puppeteer in the future. We'll likely either use Cypress E2E testing directly to test our docs, or we'll use Playwright instead (since Playwright is more lightweight and actually supports M1 machines).

There's virtually no upside to keeping Puppeteer in our dependencies - it causes issues for devs on arm64 machines and it increases CI load times installing a dependency we're not even using.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Puppeteer, script, and deps should be removed now. I wasn't 💯 that the start-test-server command in the package.json file was specific to Puppeteer, so it stayed in. Happy to remove it if it seems unnecessary.

Copy link
Member

@cee-chen cee-chen Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

start-test-server is specific to puppeteer/the a11y script, please nuke it!

Copy link
Member

@cee-chen cee-chen Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm also fairly sure we can rip out the webpack-dev-server dependency as well once that's gone; the script is the only thing using it. The wins just keep on coming 🎉 💥

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes on start-test-server but we'll need to keep webpack-dev-server to run yarn start. Adding it back now as I bump the .nvmrc.

Copy link
Member

Choose a reason for hiding this comment

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

Lol fuck I totally missed that webpack serve is powered by webpack-dev-server, I just did a basic grep for the package name 🤦 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.

Hey, I missed it too. Adding it back in a jiffy.

"test-staged": "yarn lint && node scripts/test-staged.js",
"test-cypress": "node ./scripts/test-cypress",
"test-cypress-dev": "yarn test-cypress --dev",
"test-cypress-a11y": "yarn test-cypress --a11y",
"combine-test-coverage": "sh ./scripts/combine-coverage.sh",
"start-test-server": "BABEL_MODULES=false NODE_ENV=puppeteer NODE_OPTIONS=--max-old-space-size=4096 webpack-dev-server --config src-docs/webpack.config.js --port 9999",
"yo-component": "yo ./generator-eui/app/component.js",
"update-token-changelog": "node ./scripts/update-token-changelog.js",
"update-changelog-manual": "node -e \"require('./scripts/update-changelog').manualChangelog('${npm_config_release}')\"",
"start-test-server-and-a11y-test": "cross-env WAIT_ON_TIMEOUT=600000 start-server-and-test start-test-server http-get://localhost:9999 test-a11y",
"yo-doc": "yo ./generator-eui/app/documentation.js",
"yo-changelog": "yo ./generator-eui/changelog/index.js",
"release": "node ./scripts/release.js",
Expand Down Expand Up @@ -99,7 +96,6 @@
"vfile": "^4.2.0"
},
"devDependencies": {
"@axe-core/puppeteer": "^4.4.2",
"@babel/cli": "^7.21.5",
"@babel/core": "^7.21.8",
"@babel/plugin-proposal-class-properties": "^7.18.6",
Expand Down Expand Up @@ -145,7 +141,6 @@
"@types/classnames": "^2.3.1",
"@types/enzyme": "^3.10.5",
"@types/jest": "^24.0.6",
"@types/node": "^10.17.5",
"@types/react": "^18.2.14",
"@types/react-dom": "^18.2.6",
"@types/react-is": "^17.0.3",
Expand Down Expand Up @@ -222,7 +217,6 @@
"prettier": "^2.8.8",
"process": "^0.11.10",
"prop-types": "^15.6.0",
"puppeteer": "^5.5.0",
"raw-loader": "^4.0.1",
"react": "^18.2.0",
"react-16": "npm:react@^16.14.0",
Expand Down
106 changes: 0 additions & 106 deletions scripts/a11y-testing.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion scripts/deploy/build_docs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

set -e

NODE_IMG="docker.elastic.co/eui/ci:5.3"
NODE_IMG="docker.elastic.co/eui/ci:5.5"

# Compile using node image
echo "Building docs using ${NODE_IMG} Docker image"
Expand Down
32 changes: 11 additions & 21 deletions scripts/docker-ci/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# https://github.com/zenato/docker-puppeteer/blob/master/Dockerfile

# >=12.0 required (for cypress). v18 is LTS.
FROM --platform=linux/amd64 node:18.17.1-slim
FROM --platform=linux/amd64 node:18.18.1-slim

SHELL ["/bin/bash", "-o", "pipefail", "-c"]

Expand All @@ -15,9 +15,7 @@ RUN apt-get update \

ENV APT_KEY_DONT_WARN_ON_DANGEROUS_USAGE=1

# Install latest chrome package.
# Note: this installs the necessary libs to make the bundled version of Chromium that Pupppeteer
# installs, work.
# Install latest chrome package and update libs
RUN apt-get update \
&& apt-get install -y wget xvfb ca-certificates gnupg \
--no-install-recommends \
Expand All @@ -31,24 +29,16 @@ RUN apt-get update \
&& rm -rf /var/lib/apt/lists/* \
&& rm -rf /src/*.deb

# Uncomment to skip the chromium download when installing puppeteer. If you do,
# you'll need to launch puppeteer with:
# browser.launch({executablePath: 'google-chrome-unstable'})
# ENV PUPPETEER_SKIP_CHROMIUM_DOWNLOAD true

# Install puppeteer & cypress so they are available in the container.
RUN yarn add puppeteer
RUN yarn global add cypress

# Add puppeteer user (pptruser).
RUN groupadd -r pptruser && useradd -r -g pptruser -G audio,video pptruser \
&& mkdir -p /home/pptruser/Downloads \
# Add node user to audio and video groups to enable Cypress video
# See https://wiki.debian.org/SystemGroups
# Add directory for Cypress to write screenshots and videos
RUN usermod -a -G audio,video node \
&& mkdir -p /cypress/screenshots \
&& chown -R pptruser:pptruser /home/pptruser \
&& chown -R pptruser:pptruser /node_modules \
&& chown -R pptruser:pptruser /cypress
Comment on lines -47 to -49
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 confident this change is 1:1 - in particular -G audio,video seems potentially odd that we've lost it. Do you mind explaining how/why this works, and also pushing up a Cypress test change to fail deliberately so we can ensure that screenshots/artifacts are still captured?

Copy link
Contributor Author

@1Copenut 1Copenut Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I pulled the commands apart this morning with some help from the Linux man pages. To the best of my understanding we're safe to remove the commands as are. the -G audio,video was being used to assign the pptruser to the audio and video groups to enable audio and video recordings.

Cypress does allow video recordings, but the feature is turned off by default. If we want to start video recording in the future, we'd need to add back the -G audio,video to the node user.

I've pushed a commit d606b47 that purposely fails a Cypress test. The should output a screenshot Docker image captures screenshots and they're uploaded by the Buildkite runner under the Artifacts tab.

Copy link
Member

Choose a reason for hiding this comment

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

Cypress does allow video recordings, but the feature is turned off by default. If we want to start video recording in the future, we'd need to add back the -G audio,video to the node user.

Let's do that pre-emptively please. I'd like the ability to quickly set Cypress in CI to emit videos for easier debugging if necessary via cypress.json without having to know linux/docker settings or getting confused as to why no videos are being output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

&& mkdir -p /cypress/videos \
&& chown -R node:node /cypress

# Run user as non privileged.
USER pptruser
# Run as non-privileged node user for improved security
# See https://github.com/nodejs/docker-node/blob/main/docs/BestPractices.md#non-root-user
USER node
1Copenut marked this conversation as resolved.
Show resolved Hide resolved

CMD ["google-chrome-stable"]
4 changes: 2 additions & 2 deletions scripts/test-a11y-docker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const { execSync } = require('child_process');

execSync('docker pull docker.elastic.co/eui/ci:5.3', {
execSync('docker pull docker.elastic.co/eui/ci:5.5', {
stdio: 'inherit',
});
/* eslint-disable-next-line no-multi-str */
Expand All @@ -9,7 +9,7 @@ execSync(
-i --rm --cap-add=SYS_ADMIN --volume=$(pwd):/app --workdir=/app \
-e GIT_COMMITTER_NAME=test -e GIT_COMMITTER_EMAIL=test -e HOME=/tmp \
--user=$(id -u):$(id -g) \
docker.elastic.co/eui/ci:5.3 \
docker.elastic.co/eui/ci:5.5 \
bash -c \'npm config set spin false \
&& /opt/yarn*/bin/yarn \
&& yarn cypress install \
Expand Down
4 changes: 2 additions & 2 deletions scripts/test-docker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const { execSync } = require('child_process');

execSync('docker pull docker.elastic.co/eui/ci:5.3', {
execSync('docker pull docker.elastic.co/eui/ci:5.5', {
stdio: 'inherit',
});
/* eslint-disable-next-line no-multi-str */
Expand All @@ -9,7 +9,7 @@ execSync(
-i --rm --cap-add=SYS_ADMIN --volume=$(pwd):/app --workdir=/app \
-e GIT_COMMITTER_NAME=test -e GIT_COMMITTER_EMAIL=test -e HOME=/tmp \
--user=$(id -u):$(id -g) \
docker.elastic.co/eui/ci:5.3 \
docker.elastic.co/eui/ci:5.5 \
bash -c \'/opt/yarn*/bin/yarn \
&& yarn cypress install \
&& NODE_OPTIONS="--max-old-space-size=2048" npm run test-ci \
Expand Down
2 changes: 1 addition & 1 deletion src/components/delay_hide/delay_hide.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export class EuiDelayHide extends Component<
countdownExpired: this.props.hide,
};

private timeoutId?: number;
private timeoutId?: ReturnType<typeof setTimeout>;

componentDidMount() {
// if the component begins visible start counting
Expand Down
25 changes: 12 additions & 13 deletions wiki/contributing-to-eui/testing/automated-accessibility-testing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,28 @@
# Accessibility testing
1Copenut marked this conversation as resolved.
Show resolved Hide resolved

The goal is for the entirety of the EUI docs site to be run through [axe](https://www.deque.com/axe/).
As of [#2569](https://github.com/elastic/eui/pull/2569), all of the Guidelines pages are tested and the plumbing is in place to add the rest.
The goal is for all EUI components to be run through [axe](https://www.deque.com/axe/).
Please refer to [issue #6300](https://github.com/elastic/eui/issues/6300) to see components under test.

## What is axe?

axe is an "[accessibility engine for automated Web UI testing](https://github.com/dequelabs/axe-core)".
Automated tests cover ~30% of accessibility requirements but, ~60% of accessibility bugs are caught by automated tests.
So, though it can't replace manual testing, it's a great baseline for all of our components to meet.
Automated tests cover ~30% of accessibility requirements but ~60% of accessibility bugs are caught by automated tests.
Automated testing can't replace manual testing, but it's an important baseline for all of our components.

## How to run the tests?
## How to run the Cypress accessibility tests?

* `start-test-server-and-a11y-test` runs the test suite against the entire docs site and manages it's own local server for it.
* `test-a11y` can be used if you want to run it against your dev server (assumed to be `http://localhost:8030`).
* `test-cypress-a11y` runs the accessibility test suite in a local headless Cypress environment.
* `test-a11y-docker` runs the test suite in the EUI team's CI Docker container.
* `test-cypress-dev` runs the test suite using your dev server (assumed to be `http://localhost:8030`) and the Cypress test runner.

### How to run it against 1 component?
### How to run accessibility tests against 1 component?

Though it's not setup to be run this way, there are two ways to do it.

The recommended route is to install the axe addon (for [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd) or [Firefox](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/)).
Navigate to any page and run the analyzer from your browser's dev tools.
The recommended route is to install the axe addon (for [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd), [Firefox](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), or [Edge](https://support.microsoft.com/en-us/microsoft-edge/download-the-new-microsoft-edge-based-on-chromium-0f4a3dd7-55df-60f5-739f-00010dba52cf)).
Navigate to any page and run the analyzer from your browser's developer tools.
This will return the same* results to you while also giving you some convenience utilities like highlighting the exact element that's failing.

Not as nice of a experience though potentially more direct, in `scripts/a11y-testing.js` you can modify the list of component pages return from `docsPages()` to run only one file. But remember not to check in these changes!

\* It might not actually be the same in a couple cases (e.g., we've disabled some rules or a recent update that was pushed to the addon but we haven't updated yet) but it will generally be more strict than we are, so you should never see something in CI that you can't see in the addon.

## Deconstructing an error message
Expand All @@ -48,4 +47,4 @@ Any violations should be confirmed using the [axe browser plugin](https://deque.

## Testing environment

This testing suite runs in a Docker container built and maintained by the EUI team and published to the Elastic Container Registry. Any environment-related failures should be investigated starting with the Docker directory located at [`scripts/docker-puppeteer/`](../scripts/docker-puppeteer/README.md).
This testing suite can be run on your local machine or in a Docker container. The EUI team has a plan to run these tests in a scheduled Buildkite job using the CI Docker container.
Loading
Loading