slate icon indicating copy to clipboard operation
slate copied to clipboard

Slate Testing: refactor testing setup

Open horacioh opened this issue 2 years ago • 17 comments

Description A clear and concise description of what this pull request solves. (Please do not just link to a long issue thread. Instead include a clear description here or your pull request will likely not be reviewed as quickly.)

Issue Fixes: (link to issue)

Example A GIF or video showing the old and new behaviors after this pull request is merged. Or a code sample showing the usage of a new API. (If you don't include this, your pull request will not be reviewed as quickly, because it's much too hard to figure out exactly what is going wrong, and it makes maintenance much harder.)

Context If your change is non-trivial, please include a description of how the new logic works, and why you decided to solve it the way you did. (This is incredibly helpful so that reviewers don't have to guess your intentions based on the code, and without it your pull request will likely not be reviewed as quickly.)

Checks

  • [x] The new code matches the existing patterns and styles.
  • [x] The tests pass with yarn test.
  • [x] The linter passes with yarn lint. (Fix errors with yarn fix.)
  • [x] The relevant examples still work. (Run examples with yarn start.)
  • [ ] You've added a changeset if changing functionality. (Add one with yarn changeset add.)

horacioh avatar Apr 06 '22 07:04 horacioh

⚠️ No Changeset found

Latest commit: 80965527e6791ebf410f86a57d5ab2e8634978ca

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Apr 06 '22 07:04 changeset-bot[bot]

hey folks!!

I'm going to ask some questions here to get some feedback about what is the desired outcome for the overall tests setup. hope y'all can help me make what we all expect and want for the library to be better!!

1st question: Should we have a testing setup for each package or just one for the whole repo?

I believe each package should have its own testing strategy and setup for the sake of code organization and separation. we can have common setup files and extend them from each package, but each should have its own setup. wdyt?

@dylans @bryanph @nemanja-tosic @mwood23

horacioh avatar Apr 07 '22 12:04 horacioh

Should we have a testing setup for each package or just one for the whole repo?

I think each package is necessary, but ideally aligned where possible of course.

dylans avatar Apr 07 '22 17:04 dylans

Another Question: Do we want to have some tests running on JSDOM?

If so, we might want to instead of using Jest for it, we can use Vitest since is faster. Vitest can run unit tests too so we can unify all mocha tests and JSDOM tests in one.

I'm not sure what would be the clear benefit of having JSDOM tests taking in cosideration that we have Cypress component testing, would love to know if there's any!

Another Option could be to just use cypress and mocha for all types of tests, since Cypress can run unit tests too

let me know what y'all think!!

horacioh avatar Apr 08 '22 09:04 horacioh

should this change is a minor or a patch?

horacioh avatar Apr 08 '22 13:04 horacioh

@thesunny hey! 👋🏼

regarding this message, can you ellaborate more on what type of tests are you thinking about?

thanks in advance!

horacioh avatar Apr 08 '22 14:04 horacioh

@horacioh

Should we have a testing setup for each package or just one for the whole repo?

Some might be shared, others have their own testing strategy (like slate and slate-react have different testing strategies). I do think moving the cypress component tests under slate-react might be nice as it's really just testing that package. But since it's right now separate under site/ maybe we can wait with that for now.

Do we want to have some tests running on JSDOM?

I suggest we try out the cypress component tests and see how far that gets us. If it turns out to be a performance problem at some point we could write some tests separately in a JSDOM environment if needed. As for vitest, I'd vote against it for now as the speed isn't really an issue for unit tests at the moment and it would bring in more dependencies from vite.

As for android tests, that will have to be using different tooling as cypress does not provide this unfortunately.

bryanph avatar Apr 08 '22 15:04 bryanph

Just letting a note here I've got good results with https://storybook.js.org/blog/interaction-testing-with-storybook/, not sure how it compares to cypress component testing though.

zbeyens avatar Apr 08 '22 15:04 zbeyens

@horacioh thanks for all the work your putting into testing!

FYI, I have a test setup that I'm fairly confident in for testing Slate core (not the React) using Jest. I'd like to share that with you as I've been able to test everything I've needed and it's just a few lines of code. It's easy to understand, lightweight and flexible. I'd like to review it first though as right now it's specific to the Editor I'm working on although I don't see any obvious issues with making it generic.

With respect to Android testing, the tests I was referring to are a set of manual tests I used to make sure Android support was working. Often, when I fixed one edge case, it would break the other. The tests were mostly about:

  • splitting
  • merging
  • using every input method (IME, gesture, autocorrect, autocomplete)
  • backspacing
  • key repeats

They can be found in the repo still under 0.47.x in the demo/example.

Unfortunately and admittedly, I don't know how to test this.

I do know that it has to be on a real device as the virtual device behaves differently from the real one. I also know that there are differences between every version of Android. So 8 was different from 9, 10, 11, 12, etc. That said, we can be somewhat picky in only supporting more recent versions.

I also do know that there are testing services out there that allow us to test on real Android devices. I don't have much experience with that myself though.

Sorry, not sure if that was all that helpful. I'd be happy to consult on the parts I'm familiar with but setting up the real device testing is something I don't have experience with.

thesunny avatar Apr 10 '22 23:04 thesunny

thanks @thesunny for the input!

I'm wondering if applying for a free BrowserStack license for Slate could be useful here? https://www.browserstack.com/open-source?ref=pricing

@zbeyens thanks for the link! I will contact Dominic for some questions because if we can use this with Vitest, this will be the winned imho. because we will be able to unify all the testing setup with Vitest, simplify a bunch of things and make tests run fast!

horacioh avatar Apr 11 '22 07:04 horacioh

I got this message back from the ChromaUI team (the company behing Storybook):

Nice! Slate is pretty dope. We typically sponsor community-led component libraries and design systems with 35k snapshots/month ($1,788 annual value) for free. Is that something you and your community would be interested in?

Relevant details: 
- You'll get the Open Source Starter plan for free, with usage up to 35k snapshots/month along with every other benefit of a premium plan.
- You'll be limited to 35k snapshots until you (optionally) enter a credit card to charge for any overages.
- You'll continue to receive 35k free bonus snapshots should you choose to upgrade your plan later on.
- We'll ask you to add a link to Chromatic in your project's README in return.

Let me know if you'd like to accept this offer. Meanwhile, in case you haven't setup Chromatic yet, you can get started with our Free plan right away.

I believe we can take advantage of this if we decide Storybook component testing is what we want. I'm making some examples with this so we can compare it with the current Cypress setup.

if there's anyone with experience with Storybook testing please let me know! or any related feedback.

horacioh avatar Apr 11 '22 11:04 horacioh

As a quick note, I'm following along but haven't said much yet. Testing is a challenge to get right. My perspective is mostly aligned with the comments in https://github.com/ianstormtaylor/slate/pull/4903#issuecomment-1076373954 and I'm just sort of waiting to see what emerges from the various PRs and trying to avoid my own bias.

dylans avatar Apr 14 '22 13:04 dylans

hey folks!!

let me do a quick review on what's the state of these:

  • tests rely on mocha/chai for unit tests and Cypress for Component testing (CT)
  • all the example tests are migrated to use Cypress CT
  • all slate-react tests are migrated to use Cypress CT
  • all test scripts are updated to run the latest tests
  • we have just one cypress config, since we have also one build config. it did not make too much sense to separate test scripts per package if we have the build setup unified too.
  • we are not using slate-test-utils for testing, since all the tests are running in a real browser.

I believe relying on Cypress CT would be a better solution for Slate since it relies on real browsers to make sure things are working. There's no E2E tests now, not sure how much value is bringing to the library now that we are running the tests in a very similar (faster) way.

There are also other things that might be interesting to try/decide:

  • use Storybook: Dunno how important it would be to have visual regression testing for Slate, but we can try Storybook Interaction Testing instead of Cypress CT. The upside about this is that we can have a very generous plan of Chromatic. This means that we need to rewrite the tests to Storybook (not a huge deal imho). Plate does use Storybook too, and since is the biggest Slate-powered library, that gives us clues of its value.
  • Based on the feedback from the folks working with Android, we might need an automated system that tests slate againts real devices. we can get also a feee license of BrowserStack for that. That means that we might need to have special tests for it, but it could be scoped to just Android and the known issues that can be replicated in a headless environment only.
  • adopt slate-test-utils inside Slate: AFAIK, @mwood23 is not actively working with Slate anymore, and the utils he developed in the library might be useful for others inside slate itself. seems like a great addition to the Slate ecosystem.
  • prepare the setting for modern systems: This is the more abstract of the points tbh, but I would love to have a setting in which we get the velocity and DX of Vitest and get also all the benefits of using something like Storybook for not only showing the library but also test, accessibility, playground and visual regression automations. After soem chats with the Storybook team, their test-runner is based on Jest and playwright, but they are working on making it work with vitest. Playwright is also working on a component testing setup, which brings another contestant to the whole testing system.

From now, I'm not sure what to do next apart from adding more tests to slate-react and keey the current setup for now. I'm curious about what y'all think and in which direction we should go.

Thanks for your time and let's decide the best for the library!

horacioh avatar Apr 14 '22 13:04 horacioh

also i want to point out the work @JonasKruckenberg did on this PR: #4934 you can have a taste of what is like to work with vitest by running his PR.

horacioh avatar Apr 14 '22 17:04 horacioh

not sure if someone has the time to review this after the last changes. feel free to give some feedback and any recommendations!

horacioh avatar May 08 '22 20:05 horacioh

not sure if someone has the time to review this after the last changes. feel free to give some feedback and any recommendations!

I'll admit I'm feeling a bit of analysis paralysis between the three open testing PRs, #4903 #4928 and #4934 .

dylans avatar May 08 '22 23:05 dylans

I'll admit I'm feeling a bit of analysis paralysis between the three open testing PRs, #4903 #4928 and #4934 .

sorry to hear! :( I'd try to do a better job explaining it. I understand that right now is hard to "decide" since is not well defined what are the next steps. In a couple of days I will be able to explain it better!

horacioh avatar May 09 '22 08:05 horacioh

I think the consensus that emerged has been to use Playwright for E2E tests, and testing-library + mocha for unit tests. I would not be opposed to using Storybook for component level tests.z

This has mostly meant not using Vite/vitest or Cypress. Closing this out for now in favor of what we've been using in main.

dylans avatar Dec 16 '23 11:12 dylans