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

[docs] Explain checks in Contributing #18033

Merged
merged 7 commits into from
Oct 27, 2019
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 77 additions & 3 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,16 @@ The core team is monitoring for Pull Requests. We will review your Pull Request

### How to increase the chance of being accepted?

We run a series of checks automatically when a Pull Request is opened (CI). If you're not
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
sure you can always open a Pull Request and the GitHub UI will display a serious of checks
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
that will run. If one of them fails check out the section about [Checks and how to fix them](#checksfix).
eps1lon marked this conversation as resolved.
Show resolved Hide resolved

Make sure the following is true:

- The branch is targeted at `master` for ongoing development. We do our best to keep `master` in good shape, with all tests passing. Code that lands in `master` must be compatible with the latest stable release. It may contain additional features, but no breaking changes. We should be able to release a new minor version from the tip of `master` at any time.
- If a feature is being added:
- If the result was already achievable with the core library, explain why this feature needs to be added to the core.
- If this is a common use case, consider adding an example to the documentation.
- If the result was already achievable with the core library, explain why this feature needs to be added to the core.
- If this is a common use case, consider adding an example to the documentation.
- When adding new features or modifying existing, please include tests to confirm the new behavior. You can read more about our test setup in our test [README](https://github.com/mui-org/material-ui/blob/master/test/README.md).
- If props were added or prop types were changed, the TypeScript declarations were updated.
- When submitting a new component, please add it to the [lab](https://github.com/mui-org/material-ui/tree/master/packages/material-ui-lab).
Expand All @@ -85,6 +89,75 @@ Because we will only merge a Pull Request for which all tests pass. The followin
See [About TypeScript demos](#about-typescript-demos).
- The Pull Request title follows the pattern `[Component] Imperative commit message`. (See: [How to Write a Git Commit Message](https://chris.beams.io/posts/git-commit/#imperative) for a great explanation)

#### Checks and how to fix them

If any of the checks fails click on the _Details_
link and checkout the logs of the build to find out why it failed. The following
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
section gives an overview what each check is responsible for.

##### ci/codesandbox

This task should not fail in isolation. It creates codesandboxes that use the version
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
of Material-UI that was built from this Pull Request. Use it to test more complex scenarios.

##### ci/circleci: checkout

A preflight check to see if the dependencies and lockfile is ok. Running `yarn`
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
and `yarn deduplicate` should fix most of the issues.

##### ci/circleci: test_static

Checks code format and lints the repository. The log of the failed build should explain
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
how to fix the issues.

##### ci/circleci: test_unit-1

Runs the unit tests in a `jsdom` environment. If this fails then `yarn test:unit`
should fail locally as well.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
should fail locally as well.
should fail locally as well. (You can narrow the scope of tests run with `yarn test:unit -g ComponentName`.)

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is definitely important. I'll add a link to the --grep documentation. Nowadays I use e.g. "<Tab />" to avoid also running tests for Tabs. Maybe there are some hints how to grep for test filename.

Copy link
Member

Choose a reason for hiding this comment

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

Should we recommend yarn test:watch for faster repeated runs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not right now. It's broken.


##### ci/circleci: test_browser-1

Runs the unit tests in multiple browsers (via Browserstack). The log of the failed
build should list which browsers failed. If Chrome failed then `yarn test:karma`
should fail locally as well. If other browsers failed debugging might be trickier.

##### ci/circleci: test_regression-1

Renders tests in `test/regressions/tests` and makes screenshots. This step shouldn't
fail if the others pass. Otherwise a maintainer will take a look. The screenshots
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
are evaluated in another step.

##### argos

Evaluates the screenshots taken in `test/regressions/tests` and fails if it detects
differences. This doesn't necessarily mean your Pull Request will be rejected as a failure
might be intended. Clicking on _Details_ will show you the differences.

#### ci/circleci: test_types

Typechecks the repository. The log of the failed build should list all issues.

##### deploy/netlify

Renders a preview of our docs with your changes if it succeeds. Otherwise `yarn docs:build`
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
or `yarn docs:export` usually fail locally as well.

##### mui-org.material-ui (Azure Pipelines)

This task is mostly responsible for monitoring the bundle size. It will only report
the size and not fail if it exceeds a certain threshold. If it fails there's usually
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
something wrong with the how the packages or docs were built.

#### codecov/project

Monitors coverage of our tests. A reduction in coverage isn't necessarily bad but
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
it's always appreciated if we can improve it.
eps1lon marked this conversation as resolved.
Show resolved Hide resolved

#### Misc

There are various other checks done by netlify to check integrity of our docs. Click
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
on _Details_ to find out more about them.

### Testing the documentation site

The documentation site is built with Material-UI and contains examples of all the components.
Expand All @@ -106,6 +179,7 @@ Tests can be run with `yarn test`.
### Updating the component API documentation

To update the component API documentation (auto-generated from component PropTypes comments), run:

```sh
yarn docs:api
```
Expand Down Expand Up @@ -155,7 +229,7 @@ about translations](#translations).

Material-UI documents how to use this library with TypeScript.

If you are familiar with that language, write the demo in TypeScript, and only, in a *.tsx file.
If you are familiar with that language, write the demo in TypeScript, and only, in a \*.tsx file.
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
When you're done run `yarn docs:typescript:formatted` to automatically create the JavaScript version.

If you are no familiar with that language, write the demo in JavaScript, a core contributor might help you to migrate it to TypeScript.
Expand Down