Skip to content

Conversation

@austingreendev
Copy link
Contributor

Description

This PR adds root export testing to all packages. This is to help reduce incidents where we forget to add a new component/utility to the root export. Similar to #211

Detail

These tests use a glob utility to help with finding the components. Since each package is unique it was difficult to add with the standard readdir node methods.

A few of the packages (loaders/tooltips) needed some custom logic to match the expected exports, but overall the method is consistent.

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Along with the common code abstraction, consider whether it makes sense to remove the associated PR checklist item from https://github.com/zendeskgarden/react-components/blob/master/.github/PULL_REQUEST_TEMPLATE.md

@austingreendev
Copy link
Contributor Author

Along with the common code abstraction, consider whether it makes sense to remove the associated PR checklist item from

I've removed this checklist item and also added a new index.spec.js to the .template component.

@austingreendev austingreendev merged commit 8666052 into master Dec 6, 2018
@austingreendev austingreendev deleted the agreen/root-export-tests branch December 6, 2018 17:51

const mappedFiles = fileMapper(files);

expect(keys).toEqual(mappedFiles);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we really have the expect hidden away in another module? Can we have this method just return mapped files and then in each test we do the expect there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #228

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.751% when pulling b707b7c on agreen/root-export-tests into a74838b on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants