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

Run e2e tests in multiple browsers #3325

Merged
merged 4 commits into from
Mar 14, 2023
Merged

Run e2e tests in multiple browsers #3325

merged 4 commits into from
Mar 14, 2023

Conversation

carlobeltrame
Copy link
Member

@carlobeltrame carlobeltrame commented Mar 5, 2023

Fixes #3320

Matrix builds run in parallel, so the workflow in total shouldn't be slower: https://github.com/ecamp/ecamp3/actions/runs/4338242912/jobs/7574801743

@carlobeltrame carlobeltrame marked this pull request as ready for review March 5, 2023 22:19
@usu
Copy link
Member

usu commented Mar 6, 2023

Thanks.

  • Should we use a separate cache key for each matrix strategy? We now have multiple jobs trying to store back the same cache key.
  • One of the runs was hitting 429 of the cache service. Don't think this is related to point 1, but might be an issue with running too many jobs in parallel (see screenshot below).
  • The overall status of the PR still shows as "some checks haven't completed yet". The we need to adjust some status check?

image

@carlobeltrame
Copy link
Member Author

carlobeltrame commented Mar 6, 2023

  • Should we use a separate cache key for each matrix strategy? We now have multiple jobs trying to store back the same cache key.

Oh, the Cypress GitHub action needs a different cache depending on the browser setting? That is unexpected. But sure, we can use separate cache keys. But are we sure we really want to use the Cypress GitHub Action? What advantages does that give us, since we don't use the paid Cypress Dashboard? One disadvantage I see is that CI-only E2E failures are way harder to debug locally, because the setup seems to be quite different from the local (dockerized) way of running Cypress. In my experience, such CI-only failures are one of the biggest pain points with E2E testing.
I would really like to avoid making the npm-based local usage of Cypress the recommended way instead of the dockerized one, because with npm, the e2e tests are dependent on the browsers and browser versions you have installed on your system. Docker abstracts a huge chunk of setup for us, especially for the e2e tests.

  • One of the runs was hitting 429 of the cache service. Don't think this is related to point 1, but might be an issue with running too many jobs in parallel (see screenshot below).

Seems like this error would missing cache entries in the worst case, so it shouldn't affect us much. But still, I found some pointers at coursier/cache-action#131, will look into it whether we can do anything about it. As far as I can see these errors are transient, and all the "fixes" do is log the errors instead of letting them bubble up to the GitHub Actions error system.

  • The overall status of the PR still shows as "some checks haven't completed yet". The we need to adjust some status check?

The reason for this is that we have defined on the repository that some job named "Tests: End-to-end" must be successful on every PR. But this PR renames the e2e job to "Tests: End-to-end (chrome)" etc. so we need to change our GitHub Actions requirements if we want to merge this (and rebase all other open PRs after that, so they get the new browser-specific E2E test runs).

browser:
- chrome
- firefox
- edge
Copy link
Member Author

Choose a reason for hiding this comment

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

Also add electron, because that is the default browser when running e2e tests locally.

@usu
Copy link
Member

usu commented Mar 6, 2023

Oh, the Cypress GitHub action needs a different cache depending on the browser setting? That is unexpected. But sure, we can use separate cache keys.

The cache is not for cypress, the cache is used for npm and composer cache injected into our service container by docker compose. I'm not sure we need separate ones, just wanted to clarify the question. As the environment is always the same within docker, there's no problem with that. The only disadvantage is, that all 3 matrix builds will try to save back to the same cache key. So we have some unnecessary cache writes going on (maybe concurrency issues when multiple jobs try to write back to the same cache key? don't know the cache system of Github good enough though to know if this is an issue).

@carlobeltrame
Copy link
Member Author

carlobeltrame commented Mar 6, 2023

The cache is not for cypress, the cache is used for npm and composer cache injected into our service container by docker compose. I'm not sure we need separate ones, just wanted to clarify the question. As the environment is always the same within docker, there's no problem with that. The only disadvantage is, that all 3 matrix builds will try to save back to the same cache key. So we have some unnecessary cache writes going on (maybe concurrency issues when multiple jobs try to write back to the same cache key? don't know the cache system of Github good enough though to know if this is an issue).

Ah I see. We could add a cache-setup step before the e2e tests which prepares the cache entries for all of the browser runs to re-use. But to be honest, that sounds like over-engineering to me. I really expect the GitHub cache system to be able to handle multiple semi-simultaneous write requests to the same cache key.

@usu
Copy link
Member

usu commented Mar 6, 2023

But are we sure we really want to use the Cypress GitHub Action? What advantages does that give us, since we don't use the paid Cypress Dashboard?

Mainly tried it out to see if we gain some performance increase. Gut feeling would be yes (timing more in the 5min+ range whereas before we were mostly in the 6min+ range). Haven't done a detailed analysis though and it's quite difficult to say, as the volatility can be quite high.

For me it would sound strange not to use the official action, as they have already various configurations that come along which are properly tested & maintained (wait-on, various browsers, artifact upload, etc.). You can also specify a docker image, if that is want we want.

carlobeltrame added a commit to carlobeltrame/ecamp3 that referenced this pull request Mar 8, 2023
Trying to avoid potential future problems.
ecamp#3325 (comment)
@carlobeltrame
Copy link
Member Author

carlobeltrame commented Mar 8, 2023

For me it would sound strange not to use the official action

I normally would agree with using an official specific GitHub Action. But I don't really like the Cypress GitHub Action for two reasons:

  • It is optimized for usage with the paid Cypress Dashboard, which collects videos and screenshots from all runs and is supposed to make debugging easier. This is how Cypress (the company) makes money. As long as we aren't using the paid services, the Cypress GitHub Action provides only very little benefit over rolling our own (wait-on is the only special feature, and we could easily use this npm package instead of our bash+curl script if that is better maintained). To prove my point: Storing the screenshots and videos from the test runs on GitHub is documented, but handled outside of the GitHub Action.
  • Debugging e2e tests which fail only on CI can be extremely difficult. In one example at Qualix, after quite some work, I traced the problem to a minor dependency bump in mailcatcher (similar to MailHog which we are using). This tool spontaneously stopped working on GitHub Actions only, for reasons which I still don't know. This already was extremely laborious to debug, and would have been even more difficult if the e2e tests were run differently on CI and locally. And I remember a few more times before that, where I have had to SSH into a Travis runner in order to debug what exactly was going wrong in the e2e tests. SSHing into a GitHub Actions runner unfortunately is not possible, and accurately recreating the environment inside a GitHub Actions runner is ridiculously involved (e.g. downloading a 6+ GB (compressed size) docker image). In general, I have never been in a project where the e2e tests were easy to maintain.

That's why, when it comes to e2e tests, I have an aversion against short-term improvements to developer experience, which sacrifice the reproducibility when debugging. I want to minimize the number of variables at all costs for these few times when something unexpected fails only on CI.

You can also specify a docker image, if that is want we want.

I haven't looked into the docker image option of the Cypress GitHub Action. Maybe that would make it closer. But I really like explicitly seeing in the action YML, which commands are run in which order, so I can replay everything as closely as possible locally if needed.

I feel like we should discuss this at a meeting, so I'll add the label. But we can wait with it until we have a meeting where @usu and @manuelmeister are fully present.

@carlobeltrame carlobeltrame added the Meeting Discuss Am nächsten Core-Meeting besprechen label Mar 8, 2023
carlobeltrame added a commit to carlobeltrame/ecamp3 that referenced this pull request Mar 8, 2023
Trying to avoid potential future problems.
ecamp#3325 (comment)
@pmattmann
Copy link
Member

Fazit:

  • PR Ok
  • Umbau Docker (statt Cypress Action) Ok

@pmattmann pmattmann removed the Meeting Discuss Am nächsten Core-Meeting besprechen label Mar 14, 2023
@carlobeltrame carlobeltrame merged commit b28d96c into devel Mar 14, 2023
@carlobeltrame carlobeltrame deleted the e2e-browsers branch March 14, 2023 21:46
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.

Consider running e2e tests in multiple browsers
4 participants