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 some initial tests as per #147 #169

Merged
merged 30 commits into from
Dec 30, 2020
Merged

Conversation

lamplightdev
Copy link
Contributor

Hi Cory - I've had a play with the Stencil test runner and it seems to work pretty well (uses Puppeteer under the hood). I've added some tests here for the slTabGroup, slDialog and slDrawer components - mainly checking the basic moving parts behave as expected, that the component methods work, and that the events are firing at the right times.

There are two tests here that fail (on both slDialog and slDrawer, the slHide event is called twice when hiding) - I'll file a bug report.

I'm happy to continue working through the rest of the components adding similar tests if you think it's worthwhile. The test runner does have some basic support for comparing screenshots that might be useful for the visual regression tests, although it looks a bit experimental at the moment.

@vercel
Copy link

vercel bot commented Aug 11, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shoelace/shoelace/8qjpiwt5y
✅ Preview: https://shoelace-git-fork-lamplightdev-tests.shoelace.vercel.app

@claviska
Copy link
Member

Thanks, this looks awesome. Let me play with it today or tomorrow and I'll provide some feedback! (I haven't had a chance to use the built-in test runner yet.)

@claviska
Copy link
Member

These are great — the syntax is easy enough and the tests have already helped me uncovered a bug. 😆

Let's roll with it!

Running tests in parallel can lead to timeout errors - presumably because the Puppeteer Chrome instances are heavyweight and causing slowdowns?
Instead of waiting for `transitionend` events which can be brittle, instead wait for the event itself to happen before testing for it.
Switch back to running tests in parallel as the tests are now more reliable under load.
@lamplightdev
Copy link
Contributor Author

I think I'm finished with this first round of e2e tests. They should cover the basic checks for all events and methods that are currently associated with any component.

I've made an effort to make them robust (avoiding any generic waits that could prove flakey) so I'm pretty confident they could be used in any CI/CD pipeline if required.

Currently there are two failing tests (reported as bug #195).

@claviska
Copy link
Member

claviska commented Sep 4, 2020

Thanks again for tackling this!

How do you feel about moving each test into the same folder as its respective component? I feel like people (including myself) will be more apt to write/update tests if they're not disconnected from the source.

Do you know why some of the tests take so long? Not sure if this is normal Puppeteer behavior or if there's something we can do to speed it up.

image

@lamplightdev
Copy link
Contributor Author

With regard the time it takes to run these tests, I experience similar timings here. It seems that when they run in parallel (currently 8 max-workers) then that can max out the CPU which makes each test slower to run. If you switch to running the tests in series then each test is quicker, but the overall time is longer 😄. The browser window is set up and torn down for each test, so I imagine there's some overhead there too. If we look at the average time for a test - 22/119 ~ 0.2s / test, then considering that some of the tests have to wait for the slAfter* events which are only fired once the CSS transitions are finished, then I'm not sure this is too bad.

Yep, agree the test file should live with the other component files - I will make that change, and see if I can come up with a simple test for each of the components that don't currently have any.

@claviska claviska merged commit 1fb2bfe into shoelace-style:next Dec 30, 2020
@claviska
Copy link
Member

Thanks for this! A lot has changed since this PR was submitted, so I merged it and update the tests to reflect that. I'll open up an issue with what remains to be tested.

@claviska claviska mentioned this pull request Dec 30, 2020
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