materialsproject / crystaltoolkit

Crystal Toolkit is a framework for building web apps for materials science and is currently powering the new Materials Project website.
https://docs.crystaltoolkit.org
Other
135 stars 57 forks source link

Fix and test example apps #362

Closed janosh closed 9 months ago

janosh commented 11 months ago

Closes #361.

This PR uses Playwright for end-to-end testing.

I've had great experiences with Playwright on a lot of personal TypeScript projects. I think it's by far the best E2E testing tool out there. This is the 1st time I've used it in Python and it's been a pleasure. The integration with pytest is excellent and the playwright codegen for automatically generating test code works just as well as in TypeScript.

Here's what the new test_structure.py looks like running in headed mode

pytest crystal_toolkit/apps/tests/test_structure.py --slowmo 1000 --headed

https://github.com/materialsproject/crystaltoolkit/assets/30958850/48ed7b0d-5470-4319-9a6f-f0d043763548

Without slowmo, this test finishes in 2 sec:

Screenshot 2023-08-02 at 4 07 27 PM

This PR also adds E2E tests for pourbaix.py example app and runs it in CI. Several of the other example apps were failing. I fixed some of them but haven't written tests yet. The plan is to add those in subsequent PRs.

mkhorton commented 11 months ago

Thanks for taking this on @janosh. Happy to adopt Playwright.

Do you know how it overlaps with Dash testing / Selenium-based testing?

I think 99% of issues will just be caught by seeing if the example apps run at all.

Previously in this repo, I did have Percy working too, as recommended by Dash docs. It does have the nice feature that it can attach screenshots to a PR via CI when tests are run, along with visual diffs. I'm not attached to Percy, but the feature itself may be nice for contributors. I'm not sure if Playwright offers something similar?

mkhorton commented 11 months ago

Just took a look at the test_structure.py, looks great!

mkhorton commented 11 months ago

Answered my own question re. screenshots. Not sure the best way to integrate with a PR however.

janosh commented 11 months ago

Thanks for taking this on @janosh. Happy to adopt Playwright.

That's great!

Do you know how it overlaps with Dash testing / Selenium-based testing?

I think Playwright offers a strict superset of Selenium functionality but I could be wrong. I've tried using Selenium twice and had a harder time with it. I think Playwright is another one of those Microsoft products like VS Code and GitHub where they really hit the sweet spot both in terms of ease of use and feature set.

Just took a look at the test_structure.py, looks great!

Thanks! I think we should make it a requirement for future contributions to be covered by Playwright tests? What do you think? Happy to document this in the readme and PR template.

mkhorton commented 11 months ago

I think we should make it a requirement for future contributions to be covered by Playwright tests? What do you think? Happy to document this in the readme and PR template.

As long as we provide a very minimal template (e.g., an acceptable test is copy+pasting the template and pasting in your own component), I'd definitely be onboard with this.

mkhorton commented 9 months ago

@janosh Any guidance on when this might be merged? I'd like to make sure I don't introduce any merge conflicts with #369 and it might be good to have this merged first so that we can be confident it doesn't introduce any breaking changes.

Edited to add: Also, no problem if this is not expected to merge for a while--I can proceed with #369, but it may mean some merge conflicts introduced here instead.

janosh commented 9 months ago

@mkhorton I think this is ready to go. New E2E tests for structure.py and pourbaix.py are passing locally and (hopefully) in CI following 3147802.

mkhorton commented 8 months ago

Thanks for merging!