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

Add E2E Test of the live preview feature by Playwright #131

Merged
merged 13 commits into from
Dec 15, 2022

Conversation

yuiseki
Copy link
Member

@yuiseki yuiseki commented Dec 3, 2022

Description

Resolve: #132
Blocked by: #129

  • Added minimal E2E testing by Playwright to prevent breaking the Live Preview feature
  • At this point, I am only testing that there is no error output to the browser console at all
  • In addition, I made sure to test for 3 patterns: maplibre, mapbox and geolonia
  • And I also made this test run on GitHub Actions
    • Dependencies of Playwright is too big, so I decide to use actions/cache in GitHub Actions
      • Using cache make 10x faster when not using cache!
  • Right now, the live preview with mapbox is broken in the latest main branch, so the test is failing as intended!
  • Various fixes to the live preview #129 has merged
    • So the test of this pull request sould be passed

IMPORTANT NOTE

  • Testing live preview with mapbox on local dev env has required to set the environment variable named MAPBOX_ACCESS_TOKEN
  • Also testing on the GitHub Actions has required to add the Environment secrets on the repository settings
  • I am not authorized to manipulate the Secret of this repository, so I need someone who is authorized to do so.

Type of Pull Request

  • Others (Adding more tests)

Verify the followings

  • Code is up-to-date with the main branch
  • No build errors after npm run build
  • No lint errors after npm run lint
  • No errors on using charites help globally
  • Make sure all the exsiting features working well
  • Have you added at least one unit test if you are going to add new feature?
  • Have you updated documentation?

Refer to CONTRIBUTING.MD for more details.

Screenshots

Image from Gyazo

@yuiseki yuiseki changed the title Testing the live preview feature by playwright Add E2E Test of the live preview feature by playwright Dec 3, 2022
@yuiseki yuiseki marked this pull request as ready for review December 3, 2022 06:48
@yuiseki yuiseki added the blocked label Dec 3, 2022
@yuiseki yuiseki changed the title Add E2E Test of the live preview feature by playwright Add E2E Test of the live preview feature by Playwright Dec 3, 2022
@keichan34
Copy link
Contributor

Thanks! This is a good idea. I'll try to find someone with access to the repository secrets (I don't either... maybe @miya0001 ?) I'll have to check about whose (what?) Mapbox account we should use to create the Mapbox key as well...

@keichan34
Copy link
Contributor

@yuiseki I've added the mapbox token as secrets.MAPBOX_ACCESS_TOKEN

@yuiseki
Copy link
Member Author

yuiseki commented Dec 5, 2022

Currently I change the status of this pull request to draft because fix and check mapbox secret.
I will soon re-request review, please wait!

@smellman
Copy link
Contributor

smellman commented Dec 8, 2022

@yuiseki You need merge main branch.

@yuiseki yuiseki marked this pull request as ready for review December 10, 2022 06:42
@yuiseki
Copy link
Member Author

yuiseki commented Dec 10, 2022

  • I've merge current main branch into this branch
  • I've made changes to use secrets.MAPBOX_ACCESS_TOKEN . Thanks @keichan34 !
  • I've made change to use actions/cache in GitHub Action for speed up this test
  • Finally, The test that had been added by this pull request have passed
  • I've updated the description of this pull request

This pull request is ready for review 🙏

- run: npx playwright install --with-deps chromium
if: steps.playwright-cache.outputs.cache-hit != 'true'
- run: npm run build
- run: chmod 777 dist/cli.js
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering why I need to chmod 777 dist/cli.js before running dist/cli.js.
If I don't do this, I get a permission error.
It's might be a bug, but I believe that should be clearly describe and fix in another issue/pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

TypeScript won't set execute permissions. I believe npm will set execute permissions to the files in the "bin" section when running npm package (or maybe npm install?). Here we can do chmod +x dist/cli.js or run it via node like node dist/cli.js. I don't think it's a big problem.

Copy link
Contributor

@keichan34 keichan34 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

- run: npx playwright install --with-deps chromium
if: steps.playwright-cache.outputs.cache-hit != 'true'
- run: npm run build
- run: chmod 777 dist/cli.js
Copy link
Contributor

Choose a reason for hiding this comment

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

TypeScript won't set execute permissions. I believe npm will set execute permissions to the files in the "bin" section when running npm package (or maybe npm install?). Here we can do chmod +x dist/cli.js or run it via node like node dist/cli.js. I don't think it's a big problem.

Copy link
Contributor

@naogify naogify left a comment

Choose a reason for hiding this comment

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

@yuiseki
Thank you! I confirmed that npm run test:e2e works correctly.

@keichan34 keichan34 merged commit 683eca1 into main Dec 15, 2022
@keichan34 keichan34 deleted the live-preview-test-playwright branch December 15, 2022 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E2E Testing the live preview feature of charites by playwright
4 participants