Conversation
tresajm
commented
Feb 9, 2026
- updated Puppeteer test to Playwright
- Added additional e2e tests
Move the Playwright runner Dockerfile and compose file under docker/ for local use. Add a shared compose runner definition and a short README to document usage. Relocate .dockerignore under docker/ and keep a symlink at the repo root.
Add package-level docker scripts for component library E2E and app E2E/VRT runs. Expose root lerna aggregators for docker test flows. Update the app-nextjs README to reflect the new docker build command and paths.
Add a Docker Compose wrapper script and a Linux override file to set UID/GID. Update compose services to keep node_modules and pnpm store in named volumes. Simplify the Playwright Dockerfile to rely on bind mounts instead of copying files. Document the new mount and volume behavior in docker/README.md. Details: - Added scripts/docker-compose.sh for OS-aware compose args. - Added docker/docker-compose.linux.yml for Linux user mapping. - Added named volumes for repo and package node_modules. - Removed COPY/chmod from docker/playwright/Dockerfile.
Summary - add a self-healing entrypoint to align container UID/GID with the host - ensure node_modules and pnpm store paths are created and owned correctly - keep compose centralized with volumes and repo bind mounts Details - wire scripts/docker-entrypoint.sh into the compose services - export PUID/PGID in scripts/docker-compose.sh for Linux hosts only - update docker docs to explain the entrypoint behavior - remove the now-unused linux override compose file
Summary - drop the linux-specific compose override - keep compose configuration centralized in docker/docker-compose.yml
Summary - document how to prune Docker volumes - list the specific node_modules and pnpm store volumes to remove
- Remove the `format:stencil` step from the Angular wrapper build to avoid rewriting autogenerated proxies during normal builds - Ignore all `stencil-generated` directories in Prettier to preserve native output across repo-wide formatting runs - Note: current working tree also includes regenerated Angular proxy files, which reflect the upstream generator output
Pass Playwright CLI args into containers via PLAYWRIGHT_ARGS for compose run. Append args only when provided and parse --/--playwright-args in the wrapper. Update README with package-script-first guidance, examples, and flow diagram.
Add missing done for the run-arg injection loop in docker-compose.sh.
Add a container-side runner script to decode forwarded Playwright args from base64. Update docker-compose services to use the runner and PLAYWRIGHT_ARGS_B64. Encode args in the compose wrapper and switch docker test scripts to pnpm. Refresh README examples and the flow diagram.
- Run Playwright E2E/VRT jobs inside the Playwright container image. - Drop Playwright browser install steps in CI since the image includes them. - Add `name:` property to each test to give them human readable names. - Document the CI container usage in the Playwright Docker README.
Set HOME to /github/home for Playwright container jobs and add an explicit safe.directory step after checkout. This avoids git dubious ownership errors in dorny/test-reporter and ensures test results are published.
Run Playwright container jobs as pwuser to match the /github/home owner and avoid Firefox launch failures when HOME is not owned by the current user.
Expose the reusable test workflow run id, upload unit JUnit results as artefacts, and publish all test reports from the CI/CD Pipeline workflow.
Move test reporting back into test.yml so check runs are created from the reusable workflow, keep the existing artefact uploads, and stop Renovate from running on every pull request.
Rename the stencil Playwright E2E check run to a human friendly label so results are easier to scan in the checks list.
Switch app-e2e-runner and app-vrt-runner to run the same workspace scripts CI uses. Removes the build-apps step from docker runners so Angular isn’t built during app e2e/vrt.
Expose dist/styles/scss subpaths in the exports map for NodePackageImporter resolution. Adds explicit theme.scss export and wildcard scss exports to support pkg: imports.
| /** | ||
| * ***** DEVELOPER NOTES ***** | ||
| * | ||
| * TESTING EVENTS | ||
| * | ||
| * By default, Stencil's `toHaveReceivedEventDetail({})` method will grab the details | ||
| * from the latest fired event of the specified type. | ||
| * | ||
| * This is powerful, but can be cumbersome in medium to large events, especially if | ||
| * you only care about a sub-set of values within an Event's details. | ||
| * | ||
| * If you only want to check a subset of event details, you can do: | ||
| * | ||
| * expect(eventSpy.events[eventSpy.events.length - 1].detail) | ||
| * .toEqual(expect.objectContaining({ changedIndex: 0 })); | ||
| * | ||
| */ |
There was a problem hiding this comment.
Hey @tresajm - do we need this note for this test? It doesn't look like eventSpy is being used throughout the aside test.
| // it('renders changes to the class names when the highlightColor is changed', async () => { | ||
| // component.setProperty('highlightColour', 'gold'); | ||
| // await page.waitForChanges(); | ||
| test('renders correct initial highlight colour', async () => { |
There was a problem hiding this comment.
| test('renders correct initial highlight colour', async () => { | |
| test('renders correct initial highlight-colour', async () => { |
| await expect(host.locator('p').filter({ hasText: 'This is content passed via prop' })).toBeVisible(); | ||
| }); | ||
|
|
||
| test('updates heading content when headingContentType changes', async ({ page }) => { |
There was a problem hiding this comment.
| test('updates heading content when headingContentType changes', async ({ page }) => { | |
| test('updates heading content when heading-content-type changes', async ({ page }) => { |
| borderLeftWidth: computedStyle.borderLeftWidth, | ||
| borderLeftStyle: computedStyle.borderLeftStyle, | ||
| paddingTop: computedStyle.paddingTop, | ||
| paddingRight: computedStyle.paddingRight, | ||
| paddingBottom: computedStyle.paddingBottom, | ||
| paddingLeft: computedStyle.paddingLeft, | ||
| marginTop: computedStyle.marginTop, | ||
| marginRight: computedStyle.marginRight, | ||
| marginBottom: computedStyle.marginBottom, | ||
| marginLeft: computedStyle.marginLeft, |
There was a problem hiding this comment.
Hey @tresajm, some of these values don't seem to be tested against, do we need all of them listed here?
|
This looks great, @tresajm! Just added a few small comments/suggestions 😄 |
smorrisods
left a comment
There was a problem hiding this comment.
This is coming along nicely, @tresajm! I've added a few comments. Let me know if you have any questions/comments.
| // expect(element).toHaveClasses(['ontario-aside', 'ontario-border-highlight--lime']); | ||
| // }); | ||
| const hasLime = await host.evaluate( | ||
| (el: any) => !!el.shadowRoot?.querySelector('aside')?.classList.contains('ontario-border-highlight--lime'), |
There was a problem hiding this comment.
I strongly discourage the type of any. We most likely know the type of el since it is probably the element type of `host..
| test('invalid highlight-colour falls back to teal', async ({ page }) => { | ||
| // Set invalid highlight-colour via attribute (this will trigger validation better than property assignment) | ||
| await page.evaluate(() => { | ||
| const el = document.querySelector('ontario-aside') as any; |
|
|
||
| // Switch to HTML content via attributes | ||
| await host.evaluate(() => { | ||
| const el = document.querySelector('ontario-aside') as any; |
There was a problem hiding this comment.
Another use of any. This type flow will be implicitly typed. Is there an error when trying to use .setAttribute() without this explicit cast?
|
|
||
| // const heading = await page.find('ontario-aside >>> h2'); | ||
| // Check the rendered border color - should still be teal (default) since 'banana' class doesn't exist in CSS | ||
| const borderColor = await host.evaluate((el: any) => { |
There was a problem hiding this comment.
There's an inconsistent mix of US and Canadian spelling. Canadian spelling is preferred for anything that doesn't require US spelling.
| const borderColor = await host.evaluate((el: any) => { | |
| const borderColour = await host.evaluate((el: any) => { // I notice `any` is used here as well. |