Pull Requests
Pull requests must be approved by at least two people. You can request review from or tag the DataBiosphere/terra-ui-working-group GitHub team on PRs for any feedback, but it is no longer required.
Unit tests are run on PR creation and follow-up PR commits. They must pass before merging. Code/Branch coverage should be 80%/80% or above for new code. A best effort is expected for existing code.
More information on unit tests
Integration tests against the staging environment will automatically be run when a branch is pushed. Currently, branch protection rules do not require integration tests to pass in order for a PR to be merged. However, the general expectation is that tests should pass before merging. In the case of flaky test failures that are apparently unrelated to the PR, re-running the tests a few times is preferred over merging with failing tests.
To rerun tests, go to the CircleCI page for the workflow (linked from "Details" in the PR status for integration tests).
Open the "Rerun" menu and click either "Rerun Workflow from Start" to rerun all steps (build, deploy PR, etc.) or "Rerun Workflow from Failed" to rerun only the failed jobs.
If you do not have permission to rerun workflows on CircleCI, login to CircleCI with your GitHub account.
Occasionally, a change in dev causes one or more tests to consistently fail. Usually these are announced in the terra-ui Slack channel. In these cases, it's ok to merge a PR with those tests failing. If an apparently unrelated test consistently fails on a PR after a few re-runs, post in the terra-ui Slack channel.
More information on integration tests
- Prefix JIRA ticket (for compliance reasons) on PR title, followed by a brief reason for the PR; e.g.
[IA-1234] Display Spark UIs - If you are adding a MixPanel event, make sure to go through the MixPanel Documentation Guide. For any questions, utilize the Slack channel
#mixpanel-help.
- Have clear JIRA ticket descriptions and acceptance criteria.
- Have clear PR descriptions:
- Include screenshots and/or videos on PR description when feasible.
- Show the old vs. new.
- Include logs of manual testing completed.
- Do not squash commits until your PR is ready to merge. This helps when reviewers check out code and pull new changes incrementally as they review.
- If you’re adding/modifying an integration test or suspect a non-test change is likely to affect test behaviour, make sure to
- run the test ~100 times before merging the PR using flake shaker, to spot and address any flakiness,
- add new tests to Slack notification configuration.
- Write unit tests, especially for new code.
- Keep the scope tight.
- Remember that smaller PRs are reviewed faster, larger PRs are reviewed exponentially slower.
- Whenever possible, make separate PRs for refactoring changes (e.g. linting rule changes/additions affecting existing code) vs. functional changes.
- Use the draft PR feature.
- If you’d like to open a PR that isn’t ready for review yet, open it as a draft PR so it’s easier for folks to be aware.
- Also put
[DRAFT]in the title of the PR. This makes it easier to immediately spot and ignore draft PRs from the notification email body.
- How to help PR makers and reviewers communicate at which stage they are?
- 👍🏼 comments when you agree with a comment and feel no need to elaborate.
- Re-request review when you’re done addressing comments so reviewers get notified.
- Conversations should be resolved only by the person who first commented when they believe the comment was addressed.
- Review and comment your own code, adding comments that are only meant to be read at review-time (See an example here. Of course, if the comment makes sense to future readers, consider adding the comment in the actual committed code itself). This should reduce the round trips (from teammate to reviewer back to teammate) which are expensive. It also lets you leverage your knowledge of your teammates and indicates that you are considerate of the concerns they would typically raise. This increases team trust and shipping velocity.
- Mention how you manually tested the PR on a locally deployed UI or a PR site. All team reviews are expected to include such testing.
- While submitting a review, clarify any expectation around what must be changed, and whether the change is required to merge the PR or it can be done after merging the PR.
- It’s okay to approve a PR without waiting for a comment to be addressed, to speed things up, when one or both of the following apply:
- You didn't consider not addressing the comment a blocker to merge the PR.
- You trust that the author will address the comment prior to merging, and if they happen to not, it wouldn't be a huge deal.
- A preview site is automatically deployed for PRs.