feat: e2e combined improvements #1998

dsmmcken · 2024-05-09T18:00:07Z

Contains several changes that all touch many snapshots. Combined to reduce snapshot churn.

The primary goal was to address webkit kerning issues caused by: microsoft/playwright#20203

Recommend to review using . vscode mode and initially collapsing all snapshot folders.

Moves e2e test to execute inside docker container so enviroment is consitent between ci and local
- changes e2e.yml to build a docker container, then uses that for each matrix test
- dockerfile remove deja vu stuff, environment is now the same and grid.tsx
- dockercompose adds service that runs the image from CI with matrix config via env variables
- Fixes dockerfile and compose to actually use the node version installed by nvm (existing code didn't actually use the version installed by nvm, only the commands inside the -i interactive session would)
  - Get rid of interactive mode and make source work
  - Sym link nvm node binaries to clobber the version installed by playwright jammy container
- Churns snapshots
Removes deprecated ButtonOld
- removed button old component
- changes to style guide to no longer display that component
- removed usage in chart filter overlay
- renames snapshots to by id instead of index for buttons
- Churns snapshots
Disable kerning and smoothing in webkit playwright tests:
- theory is webkit kerning may be non-deterministic, disabling entirely may avoid those diffs
- adds Playwright.css dynamically imported based of a env VITE_PLAYWRIGHT variable to AppRoot
- initially tried using built in https://playwright.dev/docs/test-snapshots#stylepath however, results were inconsistent as canvas grids may or may not re-render after playwright appended the styles
- Updated playwright version to 1.44
- Churns snapshots
Optimize ThemeColors section in style guide
- Makes styleguide section load faster
- Remove all the ContrastColor() which have poor performance and were being done of every swatch. Instead text next to swatch
- Move slow NormailzeColors() call in tooltip into props so it doesn't run unless tooltip is shown
- Churns snapshots
Run golden-layout tests in parallel
- I don't see a reason why these need to be serial, so might be slightly faster in parallel

BREAKING CHANGE: Removed ButtonOld component, use Button instead.

BREAKING CHANGE: Removed ButtonOld Component, use Button instead

codecov · 2024-05-09T18:04:57Z

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 46.36%. Comparing base (6cf1240) to head (a733478).
Report is 2 commits behind head on main.

Files	Patch %	Lines
...ackages/code-studio/src/styleguide/ThemeColors.tsx	0.00%	3 Missing ⚠️
packages/code-studio/src/AppRoot.tsx	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1998      +/-   ##
==========================================
- Coverage   46.39%   46.36%   -0.03%     
==========================================
  Files         673      672       -1     
  Lines       38770    38764       -6     
  Branches     9818     9819       +1     
==========================================
- Hits        17988    17974      -14     
- Misses      20729    20737       +8     
  Partials       53       53

Flag	Coverage Δ
unit	`46.36% <28.57%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…nd contrast colors

…utton_old

…n_old

mofojed

Just marking as needing change so that this doesn't merge until we cut a release/branch for V+.

.github/workflows/e2e.yml

packages/code-studio/src/AppRoot.tsx

tests/docker-scripts/Dockerfile

mattrunyon

I only reviewed the CI and docker files

tests/context-menu.spec.ts-snapshots/advanced-filters-6-firefox-linux.png

bmingles

LGTM

mofojed

Good to merge, added a comment to remove the VITE_PLAYWRIGHT_CSS junk

mofojed · 2024-05-17T21:21:37Z

packages/code-studio/src/AppRoot.tsx

+// load addional css for playwright docker tests
+if (import.meta.env.VITE_PLAYWRIGHT_CSS === '1') {
+  await import('./Playwright.css');
+}
+


We should follow up to see if we can fix this in Playwright with some sort of global hook to inject CSS to a page as it loads
Maybe we can use https://playwright.dev/docs/api/class-page#page-add-style-tag

I believe this gets statically replaced and then stripped out at build time if the condition resolves to false, so may not be an issue to leave this here. I did some experimenting, and the code does only seem to be present in the output when VITE_PLAYWRIGHT_CSS=1. It does still appear in the source maps, but this is presumably part of mapping to the original source.

bmingles · 2024-05-20T18:11:20Z

@dsmmcken tests pass locally for me now

bmingles · 2024-05-20T18:17:54Z

tests/docker-scripts/docker-compose.yml

    depends_on:
      dhc-server:
        condition: service_healthy

  web-ui-update-snapshots:
    extends:
      service: web-ui-tests
-    entrypoint: 'npm run e2e:update-snapshots -- --config=playwright-ci.config.ts'
+    entrypoint: '/bin/bash --login -i -c "npm run e2e:update-snapshots -- --config=playwright-ci.config.ts"'


It seems we can no longer pass in additional args to constrain this command e.g. before I could

npm run e2e:update-ci-snapshots styleguide

or

npm run e2e:update-ci-snapshots -- -g "some test name"

Not sure if this is the culprit, but it doesn't seem to be working anymore

This is the culprit. The params just get passed to the entrypoint, so now it's passing those params at the end of the bash command instead of in the npm command.

Might be fixed by adding $@ to the end of the npm command in the quotes

I've tried this a few different ways but can't get the $@ args to get included

bmingles · 2024-05-20T22:25:18Z

tests/docker-scripts/Dockerfile

@@ -19,28 +26,20 @@ RUN fc-list : family;
 COPY .nvmrc .

 # Set the default shell so the correct node/npm is used after install
-# https://stackoverflow.com/a/60137919
+# Based on https://stackoverflow.com/a/60137919


I think I found away to avoid the interactive shell here:

SHELL ["/bin/bash", "--login", "-c"] # Nvm appends its config to `.bashrc` if it exists, but the default `.bashrc` # bails if not in an interactive shell, which causes `nvm` to never actually get # configured. To avoid having to use `-i` interactive shell, we overwrite the # `.bashrc` file with a version that doesn't bail. # https://github.com/nvm-sh/nvm/issues/1985#issuecomment-813189002 RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.3/install.sh | bash RUN echo 'export NVM_DIR="$HOME/.nvm"' > /root/.bashrc RUN echo 'source "$NVM_DIR/nvm.sh" --install' >> /root/.bashrc RUN source /root/.bashrc RUN nvm install

Actually this would be simpler:

SHELL ["/bin/bash", "--login", "-c"] # The default `.bashrc` bails on the first line if not in interactive shell. # We can just empty it so that the nvm install below will actually run when sourcing # `.bashrc` RUN echo "" > /root/.bashrc RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.3/install.sh | bash RUN source /root/.bashrc && nvm install

… entrypoint

bmingles · 2024-05-21T21:23:24Z

tests/docker-scripts/Dockerfile

+# Now clobber the default node installed by the playwright
+# image so that the commands in docker compose entrypoint will 
+# also use the correct version of node without running in bash
+RUN ln -s $(which node) /usr/local/bin/nodejs


Kudos. This simplifies things.

bmingles

Tests are passing in CI and locally except for a timeout when I ran everything. Re-running the test in isolation passed, so I think we are good to go.

dsmmcken added 2 commits May 9, 2024 13:58

feat: remove deprecated ButtonOld component

cbf3fad

BREAKING CHANGE: Removed ButtonOld Component, use Button instead

snapshots

0e36244

dsmmcken self-assigned this May 9, 2024

dsmmcken requested a review from bmingles May 9, 2024 18:00

dsmmcken added 8 commits May 9, 2024 14:09

lol actually delete the component itself

669e0e5

update playwright version as support for stylePath is needed

1da176d

turn of kerning/ligatures in screenshots

7349bfe

use ids for button file names in styleguide test

4440b27

Optimize performance of themecolor styleguide by removing normalize a…

fada493

…nd contrast colors

yolo

5398099

... dunno

904a0c6

TESTING

3538dba

dsmmcken changed the title ~~feat: removed deprecated ButtonOld component~~ removed deprecated ButtonOld component May 13, 2024

dsmmcken added 16 commits May 14, 2024 15:56

yolo

6edfa82

yolo v2

54f08e3

mkdir yolo

e177fb6

mkdir first?

5b5600d

fml

b024148

more fml

5edd976

why?

bc32ec7

ah checkout here needed too of course

98fb3f0

?

62b86e4

report?

7fb521e

Merge branch 'dmckenzie_use_docker_playwright' into dmckenzie_purge_b…

9a161c7

…utton_old

ci true

f987f32

maybe?

3bc7c43

Merge remote-tracking branch 'origin/main' into dmckenzie_purge_butto…

ff8f85f

…n_old

whoops

6154353

try

f5e545b

mofojed requested changes May 16, 2024

View reviewed changes

mattrunyon requested changes May 16, 2024

View reviewed changes

mattrunyon reviewed May 16, 2024

View reviewed changes

bmingles reviewed May 16, 2024

View reviewed changes

tests/context-menu.spec.ts-snapshots/advanced-filters-6-firefox-linux.png Outdated Show resolved Hide resolved

dsmmcken added 3 commits May 16, 2024 17:42

fix log dump and use upload/download instead of cache

fdfbd2c

slash?

ec21722

skip folder, doesn't need it anymore

c03e907

dsmmcken requested review from mattrunyon and bmingles May 17, 2024 14:06

bmingles previously approved these changes May 17, 2024

View reviewed changes

mofojed previously approved these changes May 17, 2024

View reviewed changes

was using the wrong node version

837112e

dsmmcken dismissed stale reviews from mofojed and bmingles via 837112e May 17, 2024 21:54

dsmmcken added 2 commits May 17, 2024 18:51

fix to use the actual nvm version that we specifiy

e099b81

yolo

9422702

bmingles reviewed May 20, 2024

View reviewed changes

dsmmcken added 2 commits May 21, 2024 17:07

remove interactive mode and fix node version to be correct in compose…

82df202

… entrypoint

typos in comment

a733478

dsmmcken requested review from mofojed and bmingles May 21, 2024 21:17

bmingles reviewed May 21, 2024

View reviewed changes

bmingles approved these changes May 21, 2024

View reviewed changes

mattrunyon approved these changes May 22, 2024

View reviewed changes

mofojed approved these changes May 22, 2024

View reviewed changes

dsmmcken merged commit 99fc2f6 into main May 22, 2024
10 checks passed

dsmmcken deleted the dmckenzie_purge_button_old branch May 22, 2024 17:11

github-actions bot locked and limited conversation to collaborators May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: e2e combined improvements #1998

feat: e2e combined improvements #1998

dsmmcken commented May 9, 2024 •

edited

codecov bot commented May 9, 2024 •

edited

mofojed left a comment

mattrunyon left a comment

bmingles left a comment

mofojed left a comment

mofojed May 17, 2024

bmingles May 20, 2024 •

edited

bmingles commented May 20, 2024

bmingles May 20, 2024

mattrunyon May 20, 2024

bmingles May 20, 2024

bmingles May 20, 2024

bmingles May 21, 2024

bmingles May 21, 2024

bmingles left a comment

feat: e2e combined improvements #1998

feat: e2e combined improvements #1998

Conversation

dsmmcken commented May 9, 2024 • edited

codecov bot commented May 9, 2024 • edited

Codecov Report

mofojed left a comment

Choose a reason for hiding this comment

mattrunyon left a comment

Choose a reason for hiding this comment

bmingles left a comment

Choose a reason for hiding this comment

mofojed left a comment

Choose a reason for hiding this comment

mofojed May 17, 2024

Choose a reason for hiding this comment

bmingles May 20, 2024 • edited

Choose a reason for hiding this comment

bmingles commented May 20, 2024

bmingles May 20, 2024

Choose a reason for hiding this comment

mattrunyon May 20, 2024

Choose a reason for hiding this comment

bmingles May 20, 2024

Choose a reason for hiding this comment

bmingles May 20, 2024

Choose a reason for hiding this comment

bmingles May 21, 2024

Choose a reason for hiding this comment

bmingles May 21, 2024

Choose a reason for hiding this comment

bmingles left a comment

Choose a reason for hiding this comment

dsmmcken commented May 9, 2024 •

edited

codecov bot commented May 9, 2024 •

edited

bmingles May 20, 2024 •

edited