☂️ Improving Cosmos quality assurance

[francocorreasosa]

Problem

As we have been improving some Cosmos sections and building new features, we have unexpectedly broken some small pieces of our codebase. Some of these issues ended up being shipped as releases and we got notified by Cosmos' users about them.

In my opinion, adding automated tests will help us to get notified of this kind of errors before they are merged to master.

Proposed solution

TL;DR: Let's test/automate everything we can.

Visual testing ✅

We already have this, and it is working very well for us. I can't remember unexpected visual issues being introduced by accident.

We should work on exploring the new features of Chromatic and taking advantage of them.

Functional testing

Let's put an example for this. We have the Button component, which accepts an onClick prop with a function that will be called whenever the user clicks on the button.

<Button onClick={() => alert('Button was clicked')}>Click me</Button>

We expect that when the user clicks on the button, the () => alert('Button was clicked') will be executed and an alert will be displayed.

So we can write a test for this:

describe('Button', () => {
    it('calls the onClick handler when the users clicks on it',() => {
        // This is a mock
        const handler = jest.fn(); 

        // We pass the mock to the button as the onClick handler
        const button = shallow(<Button onClick={handler} />) 

        // Simulate an user click on the component
        button.click() 

        // Declare the expectation
        expect(handler).toHaveBeenCalled()
    })
})

shallow is part of the enzyme package. Learn more

Another example is Accessibility. For example, the Tabs component required to "link" some internal elements to improve accessibility (using attributes like aria-controls, aria-label, aria-selected and id), and those should be specified with a certain logic. So we can test that logic is being applied correctly.

Another example is #1239, we can iterate over all our components and check they have at least one automation attribute at its top node. This can help us when writing new components or doing big refactors to not forget to have those attributes in place.

Snapshot testing

Snapshot tests help us to keep an eye on the component's internal structure.

it('single element', () => {
    const checkbox = shallow(<Checkbox name="cosmos" />)

    expect(checkbox).toMatchSnapshot()
})

This will generate a file with the render of the Checkbox component. The goal is to snap the internal structure of the component with variations in the props. It is very important the components are pure for these tests to work, otherwise the snapshots won't ever be equal and the tests will always fail.

Other tests

Release checks

We can run a script at the end of each release to check the desired version is up on npm, the docs site is up to date with the latest version, the version-specific deployment is showing the desired version, etc...

Please let me know your thoughts on this :)

Moving the table to airtable because updating this markdown table is a nightmare.

Checklist: https://airtable.com/tblQ0trCtZZo9RQ8Q/viw7bmGzRgL0eikGO

Old table is here

| Component | Assignee | Visual | Custom props | Event handlers work | Automation | Accessibility | |------------------|----------------------------------|--------|--------------|---------------------|------------|---------------| | Alert | | ✅ | ✅ | ✅ | ✅ | | | Avatar | | ✅ | ✅ | ✅ | ✅ | | | Badge | | ✅ | ✅ | ✅ | ✅ | | | Breadcrumb | | | ✅ | ✅ | ✅ | | | Button | | ✅ | ✅ | ✅ | ✅ | | | Checkbox | | | ✅ | ✅ | ✅ | | | Code | | | ✅ | ✅ | ✅ | | | Heading | | | ✅ | ✅ | ✅ | | | Icon | | | ✅ | ✅ | ✅ | | | Image | | | ✅ | ✅ | ✅ | | | Label | | | ✅ | ✅ | ✅ | | | Link | | | ✅ | ✅ | ✅ | | | Logo | | | | ➖ | ❌ | | | Paragraph | | | | ➖ | ❌ | | | Select | @francocorreasosa | | | ✅ | ✅ | | | Spinner | | | | ➖ | ❌ | | | Switch | | | | ✅ | ✅ | | | Tag | | | | ✅ | ✅ | | | TextInput | | | | ✅ | ✅ | | | Text | | | | ➖ | ❌ | | | TextArea | | | | ✅ | ✅ | | | Tooltip | | | | ➖ | ❌ | | | RowLayout | | | | ➖ | ❌ | | | ColumnLayout | | | | ➖ | ❌ | | | GalleryLayout | | | | ➖ | ❌ | | | PageLayout | | | | ➖ | ❌ | | | AvatarBlock | | | | ➖ | ❌ | | | ButtonGroup | | | | ➖ | ✅ | | | DangerZone | | | | | ❌ | | | Dialog | | | | ✅ | ✅ | | | EmptyState | | | | ✅ | ✅ | | | Form | | | | ➖ | ✅ | | | FormGroup | | | | ➖ | ✅ | | | List | | | | ➖ | ✅ | | | Page Header | | | | ✅ | ✅ | | | Pagination (all) | @francocorreasosa | | | ✅ | ✅ | | | Resource List | | | | ✅ | ✅ | | | Stack | | | | ➖ | ✅ | | | Table | @francocorreasosa | ✅ | | ✅ | ✅ | | | Tabs | @siddharthkp / @francocorreasosa | ✅ | | ✅ | ✅ | ✅ |

[siddharthkp]

✨ really happy to see this

tl;dr: let's focus on functional tests and the rest will follow.

Here are my thoughts:

1. visual testing - this isn't a problem for us, that's so cool

2. functional tests -

   2.1 I fully agree with you that we need more tests.

   2.2 I'd like to make a small tweak by testing components as a user instead of testing the functionality

   example: testing if an onclick handler works is testing if react works properly. Instead if we test if a text box appears on click, we would be able to test if cosmos components play well with the application's components

   2.3 This is a great place to test the automation attributes as well. That's the handle we should use to target elements (that's what they are there for)

   2.4 In our tests/jest setup, we can set console.error to fail the build, so that we never have outdated examples/stories

   2.5 I'd also like to suggest react-testing-library instead of enzyme because it makes you think about the rendered output instead of the implementation detail

3. accessibility -

   3.1 keyboard access: I think this should be covered by functional tests as well. you want to test of the tabs switch when you hit keys. We shouldn't test the html tags because that's implementation detail

   3.2 Screen reader: I don't have a good way of testing this. Testing if tags are added does not sound useful

4. Snapshot tests - For us, snapshot tests have been more noise than signal 😞 because of the size of our codebase.

   Snapshots tests by design catch regressions and not first time mistakes which make them a bad substitute for component guidelines (like writing pure functions)

   Are there any use cases where snapshot tests will add to the functional tests we write?

   Caveat: They serve as a good code review tool, you can review snapshots instead of code to see if there are unintentional changes. (we can use them for reviews by adding an update in precommit)

Please disagree loudly, getting testing right is going to set up for success for a long time 🙂

[landitus]

Other thing missing for tests that is important is cross-browser testing. I believe chromatic will introduce them soon, but still is something to take care of (we had some tiny issues).

[landitus]

Another part of testing, is how we recommend users to test apps that use Cosmos components (which is related to 2.2). So far, teams had issues with their snapshots tests because of the ever changing classnames, for example.

We can document what's the recommended way so that teams don't run into trouble.

[siddharthkp]

Is this an umbrella issue to keep open?

[siddharthkp]

Here's a list of components (to help divide)

I'd say just claim one here and start working on it. The goal is not to step on each others toes.

Key

Visual - Does this component have stories for all use cases (including soft deprecated API) Custom props - Does this component support adding a custom prop like id or margin Event handlers work - Do the event handlers actually work (not just called) Automation - Does this component have the right automation attribute Accessibility - Is this component keyboard accessible (we should start with keyboard, we need to do some research on screen reader)

If there is something missing, please add it

Added: ✅ Failing: ❌ (example: component not accessible) Not application: ➖(example: Paragraph does not have accessibility tests, does not have callback handlers)

(or anything you want)

(moved table to issue's description)

[francocorreasosa]

I like the idea @siddharthkp! I've assigned myself some components I already have in my head

[siddharthkp]

I think we'll have to test the automation attributes a little more closely. Example: Avatar doesn't have an automation attribute at all 😄

[siddharthkp]

Can I suggest a different approach here:

I was working on the custom prop test for Alert, and it was super easy to extend that to another component (Avatar)

It's faster to scale this horizontally across all components. I was able to quickly achieve these 3 things:

1. Add test for custom prop
2. Use automation attribute (and therefore test that as well)
3. Add a fixture that can be extended and used for other tests

Here's the work in progress pull request: https://github.com/auth0/cosmos/pull/1308

I'm sure I can cover all the components in 2 days creating a nice base for others. @francocorreasosa what do you think?

It does require touching component files to fix the bug, so merge conflicts coming up 😢

[francocorreasosa]

I think we'll have to test the automation attributes a little more closely. Example: Avatar doesn't have an automation attribute at all 😄

Sorry about that, I thought we had them all covered. I updated the table to reflect the actual situation.

I'm sure I can cover all the components in 2 days creating a nice base for others. @francocorreasosa what do you think?

Go ahead, it's going to be faster than covering each one at the time.

[francocorreasosa]

I'm going to work on the event handlers tests then, maybe we can get a similar result in speed increase if we work it this way :)