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

Rewrite older Puppeteer tests to use page.$eval instead of page.evaluate #4967

Open
querkmachine opened this issue May 2, 2024 · 0 comments
Labels
awaiting triage Needs triaging by team tech debt

Comments

@querkmachine
Copy link
Member

Cause

Currently, the Puppeteer tests we run against JavaScript functionality use two different coding styles when evaluating the page's content.

One of them is page.evaluate, which looks similar to this:

it('renders the button thing', async () => {
  await render(page, 'component', examples.default)
  const buttonThingType = await page.evaluate(() => document.body.querySelectorAll('.button-thing').type)
  expect(buttonThingType).toBe('button')
})

We also use page.$eval in many places:

it('renders the button thing', async () => {
  await render(page, 'component', examples.default)
  const buttonThingType = await page.$eval('.button-thing', (el) => el.type)
  expect(buttonThingType).toBe('button')
})

Consequentially, we have a mixture of the two styles floating around, with different components favouring one style over the other. Many components have both styles within the same test suites.

There are 87 instances of components using page.evaluate. The components using it the most include:

  • Accordion
  • Button
  • Checkboxes
  • Radios
  • Tabs

There are 126 instances of components using page.$eval. The components using it the most include:

  • Character Count
  • Exit This Page
  • Header
  • Password Input
  • Skip Link

These components use a fairly even split of both styles:

  • Error Summary
  • Notification Banner

Consequences

This is inconsistent, and inconsistency sucks when it comes to code reuse, onboarding new team members and contributors, and general code neatness.

As the page.$eval style seems to have been used more recently, I assume this is the style that we would favour should we rewrite the older tests.

Impact of debt

Low

Reason (impact of debt)

The older style tests still work and will probably work for the indefinite future.

This tech debt only affects the internals of the team, and does not affect our users outside of potential contributors.

Effort to pay down

Medium

Reason (effort to pay down)

Quite a lot of tests would need to be rewritten, although the differences aren't too significant. Mostly it requires an investment of time, rather than hard think-y effort.

Overall rating

Low

Reason (overall rating)

There's no immediate need or benefit to doing this outside of maintaining glorious consistency.

@querkmachine querkmachine added awaiting triage Needs triaging by team tech debt labels May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting triage Needs triaging by team tech debt
Projects
None yet
Development

No branches or pull requests

1 participant