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

feat: editor e2e #378

Merged
merged 10 commits into from
Jan 9, 2025
Merged

feat: editor e2e #378

merged 10 commits into from
Jan 9, 2025

Conversation

deltork
Copy link
Collaborator

@deltork deltork commented Jan 7, 2025

PR Goal?

Add tests for the studio editor

Fixes?

n/a

Feedback sought?

readiness check

Priority?

high

Tests added?

How to test?

  • Checkout this branch
  • Run studio-cli on your local machine
  • Run npx nx run-many --targets=serve,serve-fr,serve-es --projects=studio-web in the local branch
  • Wait until your browser opens, and you can confirm that studio is up and running
  • Run the tests npx nx e2e studio-web
  • Wait for the report

Test notes

  • Firefox tests are currently not 100% stable
  • Mobile Chrome test of alignment change is flaky in GitHub ci
    image

Confidence?

high

Version change?

no, all changes are to support end-to-end testing

@deltork deltork requested review from joanise and roedoejet January 7, 2025 15:10
Copy link
Contributor

github-actions bot commented Jan 7, 2025

PR Preview Action v1.5.0
Preview removed because the pull request was closed.
2025-01-09 20:20 UTC

@joanise
Copy link
Member

joanise commented Jan 7, 2025

We should document somewhere that we need to run npx playwright install --with-deps firefox chromium webkit before running npx nx e2e studio-web for the first time.

@joanise
Copy link
Member

joanise commented Jan 7, 2025

And also that npx nx e2e studio-web tests Studio-Web. The logical place is ### Testing/#### Studio-Web in the top-level README.md.

export const disablePlausible = async (page: Page) => {
//disable plausible
await page.evaluate(
async () => await window.localStorage.setItem("plausible_ignore", "true"),
Copy link
Member

Choose a reason for hiding this comment

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

nice and simple

@@ -1,9 +1,10 @@
import { test, expect } from "@playwright/test";
import { testText, defaultBeforeEach } from "../test-commands";
import { testText, disablePlausible } from "../test-commands";
Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks for adding this!

Copy link
Member

@joanise joanise left a comment

Choose a reason for hiding this comment

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

Ready to merge, except for the top-level README.md documenting how to run these tests.

@deltork
Copy link
Collaborator Author

deltork commented Jan 8, 2025

npx playwright install --with-deps firefox chromium webkit

It is part of the husky/post-install process. Should I make it post-checkout instead?

@deltork deltork force-pushed the dev.del/feat-editor-e2e branch from 5cd5d9c to 9614658 Compare January 8, 2025 21:52
@joanise
Copy link
Member

joanise commented Jan 8, 2025

The updated readme looks nice, thanks!

@deltork deltork force-pushed the dev.del/feat-editor-e2e branch from b7a86ab to 8fb71e5 Compare January 9, 2025 16:16
@deltork deltork requested a review from joanise January 9, 2025 16:44
@joanise
Copy link
Member

joanise commented Jan 9, 2025

npx playwright install --with-deps firefox chromium webkit

It is part of the husky/post-install process. Should I make it post-checkout instead?

Post install seems like the right time, as long as it's also documented (as you now did) because if I update an existing sandbox where I did the install before this was added, then I won't have done it.

Copy link
Member

@joanise joanise left a comment

Choose a reason for hiding this comment

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

The looks good to me.

The only thing I disagree with is going to 9 shards: we spend about 3 minutes in each shard setting up (install, spin up, etc), and in each shard we do at most 1 minute of work. It just feels like wasted resources. I would go back to 4 shards, maybe even 3, to keep the ratio of setup to work more reasonable. I know this will mean waiting a couple minutes longer for CI to finish, but I don't like all those CI minutes just spent installing.

Is it possible to have multiple threads in the same shard?
According to https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories the standard runner has 4 CPUs, so maybe it's possible to have just one job doing all the installation just once, and have 4 separate processes in the same job doing all the shards?

@joanise
Copy link
Member

joanise commented Jan 9, 2025

so maybe it's possible to have just one job doing all the installation just once, and have 4 separate processes in the same job doing all the shards?

Yes, I believe it is. I just tested and this works in CI:

      - name: background and wait
        run: |
          for shard in 1 2 3 4; do
            echo "shard $shard" && sleep 3 &
          done; wait
          echo "all done"

Replace the echo by the shard command, and then you could run the playwright merge reports command right after in the next step.

@joanise
Copy link
Member

joanise commented Jan 9, 2025

Oh, wait, playwriting accepts a --workers 4 option that does the same thing internally, much simpler. Never mind my manual splitting business!

@deltork deltork force-pushed the dev.del/feat-editor-e2e branch from 8fb71e5 to 1813969 Compare January 9, 2025 19:39
@deltork
Copy link
Collaborator Author

deltork commented Jan 9, 2025

Oh, wait, playwriting accepts a --workers 4 option that does the same thing internally, much simpler. Never mind my manual splitting business!

I have reduced the shards to 3 and increased the workers to 4.

@deltork deltork merged commit 44b9915 into main Jan 9, 2025
6 checks passed
@deltork deltork deleted the dev.del/feat-editor-e2e branch January 9, 2025 20:20
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.

2 participants