search

woocommerce / storefront

Official theme for WooCommerce
https://wordpress.org/themes/storefront/
974 stars 470 forks source link

Use 'wp-scripts test-e2e' instead of directly calling jest #1979

Closed Aljullu closed 1 year ago

Aljullu commented 2 years ago

Supersedes #1958 and it better aligns Storefront e2e tests with what we have in WC Blocks.

How to test the changes in this Pull Request:

  1. Make sure e2e tests pass in this PR.
  2. Optionally, verify tests also pass locally: 2.1. Start docker. 2.2. Start wp-env: npm run wp-end start. 2.3. Build Storefront: npm run build. 2.4. Run tests: npm run test:e2e. 2.5. Verify all tests are a success.
Aljullu commented 2 years ago

looks good but wanted to confirm - do we still need after moving to wp-env the jest-puppeteer dependency and jest config on lines 63-71? https://github.com/woocommerce/storefront/blob/8b74758b402c5c6d32112e4134348c85c3fbf955/package.json#L63-L71

Yes, I think so. Because wp-scripts test-e2e internally calls Jest. At least, I couldn't get it to work when removing the config or jest-puppeteer.

danielwrobert commented 1 year ago

@Aljullu looks like the last activity on this PR was ~10 months ago. Is this still relevant?

I'd be happy to review it, if so. Otherwise I'll go ahead and close it out.

Aljullu commented 1 year ago

@Aljullu looks like the last activity on this PR was ~10 months ago. Is this still relevant?

Ups, I completely forgot about it! While I don't think it's urgent, I do think it's worth merging it as it makes the Storefront e2e testing infrastructure closer to other projects like WC Blocks.

danielwrobert commented 1 year ago

While I don't think it's urgent, I do think it's worth merging it as it makes the Storefront e2e testing infrastructure closer to other projects like WC Blocks.

No worries, I'll add it to my list to review today or tomorrow!

danielwrobert commented 1 year ago

Everything tests as described here all tests in this suite are passing locally. It looks like come ESLint checks are failing, however. I tried to re-run them and see if they'd pass but that didn't seem to help.

Aljullu commented 1 year ago

Everything tests as described here all tests in this suite are passing locally. It looks like come ESLint checks are failing, however. I tried to re-run them and see if they'd pass but that didn't seem to help.

Right... they don't seem to be introduced by this PR, though. So not sure if this should block merging or not. :thinking:

Aljullu commented 1 year ago

perhaps a rebase will get them sorted out?

This branch is already up-to-date. Honestly, I'm not sure why those jobs are passing in other PRs because the fails seem legit.

In any case, I will go ahead and merge this PR as-is. We can work on the failing jobs in follow-ups.