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

Introduces a Merge<> utility and a type testing pattern #1505

Merged
merged 3 commits into from
Oct 7, 2021

Conversation

jfuchs
Copy link
Contributor

@jfuchs jfuchs commented Oct 6, 2021

This PR:

  • Adds a new utility type to merge two objects
  • Introduces a pattern for testing our types
  • Refactors src/util/types.ts into separate files per utility
  • Narrows the test matching for jest to exclude type testing

I still need to:

  • verify that these tests will be run in CI
  • verify that these tests won't contribute to our built type definitions in lib edit: turns out these tests do contribute to the built type defs, but so do our unit tests, so i'm going to drop this as a requirement for now

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2021

⚠️ No Changeset found

Latest commit: acee3ab

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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2021

size-limit report 📦

Path Size
dist/browser.esm.js 55.41 KB (0%)
dist/browser.umd.js 55.73 KB (0%)

@jfuchs
Copy link
Contributor Author

jfuchs commented Oct 6, 2021

I've been wanting a pattern for testing types for a while now, and this is what I've come up with. I'm very open to feedback about how else to accomplish confidence in our types. Some questions to consider:

  • Looking at the two type test files I've added, is it clear how the file is organized? Is it clear what each test is testing for?
  • If you made a change and type compilation broke in one of these type test files, would you know what to do?
  • Could you see yourself writing type tests proactively in the course of creating or updating a component?
  • Could you see yourself writing type tests in the course of fixing a bug with our types?
  • How could the type test files be more readable?

@jfuchs jfuchs requested review from a team and colebemis and removed request for a team October 6, 2021 17:40
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

Love this! Excellent work, @jfuchs

@pksjce
Copy link
Collaborator

pksjce commented Oct 7, 2021

This is pretty cool @jfuchs! Especially liked the set of tests for a utility like Merge. It works very well. I will definitely write these if I create such utilities.
As for failures, it seems straightforward enough to understand and fix. Great early indicator instead of us finding it out during integration.
However, for components, I'd expect many of the happy paths to get covered in unit tests and storybooks. Don't you think? We could reach out to this tool to write negative tests to prevent certain known edge case errors or bugs discovered during integration. I would find it tedious to write up tests that I've already done during unit testing.

@jfuchs
Copy link
Contributor Author

jfuchs commented Oct 7, 2021

@pksjce Thanks for the thoughts! And welcome!

However, for components, I'd expect many of the happy paths to get covered in unit tests and storybooks. Don't you think? We could reach out to this tool to write negative tests to prevent certain known edge case errors or bugs discovered during integration. I would find it tedious to write up tests that I've already done during unit testing.

Great question! I agree that many of the happy paths will be covered elsewhere, and as long as CI is running typechecking against those, we'll benefit from that. And I agree that this tool would be the place for negative tests.

What I'm not sure about is whether having good coverage in our test suites and stories for a given component would necessarily mean good coverage for the types. What do you think?

edit: I'm gonna merge, since it looks like everyone is pretty open to this kind of testing in general. I still look forward to talking about how broadly we expect to use this kind of test

src/TextInput.tsx Outdated Show resolved Hide resolved
Co-authored-by: Cole Bemis <colebemis@github.com>
@pksjce
Copy link
Collaborator

pksjce commented Oct 8, 2021

🤔 Is it a pain point within the team that we are constantly running into typing errors during certain situations like integration? Or has it happened a lot that we've broken types during releases?
AFAIK, types that a component library exposes are

  • Props
  • Callback arguments
  • Callback return values
    These should get tested in tests and stories.(Ideally we should have integration testing too so that all interactions have sufficient coverage along with opportunity to automate visual testing.)
    Avoiding impossible states should be api design driven rather than only type restricted. But this is where I see most value in this tool. We can specifically test that we haven't opened up impossible states to our consumers. It's good dx and educational to an extent too.
    We should add this to our contributor docs and keep an eye out in PRs so people can be made aware to use these tests.

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.

3 participants