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

Browser testing #348

Merged
merged 4 commits into from
Jan 15, 2020
Merged

Browser testing #348

merged 4 commits into from
Jan 15, 2020

Conversation

ctavan
Copy link
Member

@ctavan ctavan commented Dec 3, 2019

Follow-up for #341 but still WIP!

  • Currently the examples install uuid from npm (in the current state it's even a github repo where I uploaded the npm package content just for testing, but I would change that to an npm package version). How would we run the test before releasing a new version to npm? -> Now using local dist build.
  • Define a browser matrix and run the test for the browser we want to support.
  • Maybe extract the setup and teardown code. -> for the time being I don't see much room for improvement.
  • Maybe only run this on master/next but not for PRs from forks. -> Not for now.

@ctavan ctavan force-pushed the browser-testing branch 2 times, most recently from 79dcd82 to 0fcbbf9 Compare December 4, 2019 21:20
@ctavan ctavan changed the base branch from modernize-test-tooling to next December 4, 2019 21:20
@ctavan ctavan force-pushed the browser-testing branch 3 times, most recently from b2a9f8b to e41438d Compare December 4, 2019 21:39
@ctavan ctavan force-pushed the browser-testing branch 2 times, most recently from 19a8748 to d2c0339 Compare December 4, 2019 22:26
@ctavan
Copy link
Member Author

ctavan commented Dec 4, 2019

@broofa I would be interested in some preliminary feedback on this one (it's not ready for approval yet).

Here's what it currently does:

That way we should get end-to-end assertions that uuid works as expected when used with webpack/rollup/etc. in the browser.

Open questions:

  • Currently the examples install uuid from npm (in the current state it's even a github repo where I uploaded the npm package content just for testing, but I would change that to an npm package version). How would we run the test before releasing a new version to npm?
  • What do you think about the approach of rendering uuid() output into html and the running assertions on it through selenium?

What I don't really struggle with, but still have to do:

  • Define a browser matrix and run the test for the browser we want to support.
  • Maybe extract the setup and teardown code.
  • Maybe only run this on master/next but not for PRs from forks.

@ctavan ctavan force-pushed the browser-testing branch 15 times, most recently from c6b1c54 to 836d096 Compare January 9, 2020 15:44
@ctavan
Copy link
Member Author

ctavan commented Jan 9, 2020

@broofa I think I've done quite some progress on this one now and I believe it's ready for a more serious review!

What we get here is:

  • a basic test of the module API when used with webpack and rollup
  • all tests run in a set of browser that we intend to support (minimum and latest versions)
  • tests are run in Browserstack, with all browsers running in parallel

When defining the browser support matrix I somewhat arbitrarily chose the minimum browser version that supports both let/const and crypto.getRandomValues().

Since the tests do take quite a while (~7-15 mins) to run the only thing I'm still wondering is when to execute these tests. I have also experienced that these test, when run from Github actions, sometimes time out which I haven't experienced yet when running them from my local machine.

So all in all running them on every single pull requests seems wasteful and potentially a bit brittle, after all we mostly want to ensure that we don't introduce regressions, so maybe running them before each release would be enough? If so, how would we achieve that? What do you think?

@ctavan ctavan requested a review from broofa January 9, 2020 15:55
@ctavan ctavan changed the title WIP Browser testing Browser testing Jan 9, 2020
@broofa
Copy link
Member

broofa commented Jan 15, 2020

Hey, this looks really good! Thanks for tackling this.

Add the following to .gitignore? Looks like npm run test:browser generates these.

# Browserstack
browserstack.err
local.log

So all in all running them on every single pull requests seems wasteful and potentially a bit brittle

Regarding "wasteful", do we foresee enough PR churn for that to really matter? 'Seems like the benefit of having CI tests pass as a prerequisite for merging a PR would outweigh the cost there... but, that depends on the cost. FWIW, I'm on one BS's opensource accounts so it doesn't cost me anything. That expires in April 2020, though. (I assume I can ask them to renew it, but have yet to go through that process with them.)

We could potentially see about getting an organization account of some sort I suppose...?

I'm not familiar enough with Github actions to know if there's a way to tear down a workflow if/when the PR is updated, so any BS tests in progress get cancelled and restarted? Or maybe that happens automatically? (Seems like an obvious thing to do.)

Regarding "brittle", the timeout issue is a little concerning but I think it would be better for all concerned if we figured out how to make CI testing work in the cloud rather than relying on the vagaries of individual contributor environments.

tl;dr: Triggering CI tests on each commit is nice.

@ctavan
Copy link
Member Author

ctavan commented Jan 15, 2020

Thanks for the feedback!

So let's start out by running the Browserstack-Tests on all PRs and merges just like the regular tests and let's see if we can either fix brittleness or if it doesn't even turn out to be problematic. They run on my OpenSource account as well (still valid until April 2021 I think) so they won't incur actual cost.

Re gitignore: done, thanks for finding that one!

@ctavan
Copy link
Member Author

ctavan commented Jan 15, 2020

OK, so while I was hit by a brittle browser test run (where only tests in Firefox 70 timed out for whatever reason), I figured out that it is actually faster to run the different browsers as a github actions matrix.

I was hoping that this would also allow us to re-trigger specific failed browser individually but that doesn't seem to be the case.

@ctavan ctavan merged commit 172d43d into next Jan 15, 2020
@ctavan ctavan deleted the browser-testing branch January 15, 2020 23:33
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.

None yet

2 participants