-
Notifications
You must be signed in to change notification settings - Fork 7
Feature review process
Before the ticket can be turned into a Pull request in Github the following steps have to be met.
- All Acceptance Criteria must be met.
- A visual front end check to make sure that the feature actually looks as it should according to the designs attached to the ticket
- A linter is run against the code
yarn lint - The Tests pass
yarn test - The Test Coverage is passed
yarn test --coverage
No console.log(randomTestVar); or unwanted code comments // import ScrollPane from "components/ScrollPane"; to be left in unless necessary.
A link to the Jira ticket is a must so the reviewer has easy access to the A/C. Screen grabs of what has been done can be helpful. Steps on how to build can be helpful if new approach. Steps on how to test can be helpful Make sure before you merge with the latest in Master possible before you send out the ticket
- Check to see the build has finished and that all the checks have turned green.
- Perhaps Codecov has failed or Codacy. This occasionally will stop the build.
- You must pull down the code from the necessary repos and build as instructed.
- Run the tests in each of the necessary builds.
- If the build fails due to the code coverage being below the necessary threshold run
yarn test --coverageand advise on the problem with a comment.
- Go through the A/C step by step checking that what is supposed to work works. Use data provided in the PR if it is provided.
- Test edge cases if possible
- Check through the designs if there are any to make sure the front end design is up to scratch.
- Use dev tools in Chrome to make sure elements are rendered how you would expect them.
- Chrome has extensions that can be used to measure pixel sizes and colour hexs.
- Change size of the browser window to test if anything breaks.
- Use the zoom functionality in the browser
- Test the new functionality in the PR just by using a keyboard.
The most important thing to cover in a review is the overall design of the PR. Do the interactions of various pieces of code in the PR make sense? Does this change belong in your codebase, or in a library? Does it integrate well with the rest of your system? Is now a good time to add this functionality?
Does this PR do what the developer intended? Is what the developer intended good for the users of this code? The “users” are usually both end-users (when they are affected by the change) and developers (who will have to “use” this code in the future).
Mostly, we expect developers to test PRs well-enough that they work correctly by the time they get to code review. However, as the reviewer you should still be thinking about edge cases, looking for concurrency problems, trying to think like a user, and making sure that there are no bugs that you see just by reading the code.
You can validate the PR if you want—the time when it’s most important for a reviewer to check a PR’s behavior is when it has a user-facing impact, such as a UI change. It’s hard to understand how some changes will impact a user when you’re just reading the code. For changes like that, you can have the developer give you a demo of the functionality if it’s too inconvenient to patch in the PR and try it yourself.
Another time when it’s particularly important to think about functionality during a code review is if there is some sort of parallel programming going on in the PR that could theoretically cause deadlocks or race conditions. These sorts of issues are very hard to detect by just running the code and usually need somebody (both the developer and the reviewer) to think through them carefully to be sure that problems aren’t being introduced. (Note that this is also a good reason not to use concurrency models where race conditions or deadlocks are possible—it can make it very complex to do code reviews or understand the code.) Check to see if code for accessibility especially screen readers has been added to describe components and copy. When using colours in the code make sure they are variables set in the theme file not hard coded.
Is the PR more complex than it should be? Check this at every level of the PR—are individual lines too complex? Are functions too complex? Are classes too complex? “Too complex” usually means “can’t be understood quickly by code readers.” It can also mean “developers are likely to introduce bugs when they try to call or modify this code.”
A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates might need to be solved in the future. The future problem should be solved once it arrives and you can see its actual shape and requirements in the physical universe.
Ask for unit, integration, or end-to-end tests as appropriate for the change. In general, tests should be added in the same PR as the production code unless the PR is handling an emergency.
Make sure that the tests in the PR are correct, sensible, and useful. Tests do not test themselves, and we rarely write tests for our tests—a human must ensure that tests are valid.
Will the tests actually fail when the code is broken? If the code changes beneath them, will they start producing false positives? Does each test make simple and useful assertions? Are the tests separated appropriately between different test methods?
Remember that tests are also code that has to be maintained. Don’t accept complexity in tests just because they aren’t part of the main binary.
Did the developer pick good names for everything? A good name is long enough to fully communicate what the item is or does, without being so long that it becomes hard to read.
What if the existing code is inconsistent with the style guide? Per our code review principles, the style guide is the absolute authority: if something is required by the style guide, the PR should follow the guidelines.
Otherwise, if the style guideline is more a recommendation and not a requirement, then it’s a judgment call whether the new code should be consistent with the style guide or the surrounding code. Bias towards following the style guide unless the local inconsistency would be too confusing.
Does the ticket contain components that are not in the Style Guide. Should they be added?
If no other rule applies, the author should maintain consistency with the existing code.
Either way, encourage the author to file a bug and add a TODO for cleaning up existing code.
Look at every line of code that you have been assigned to review. Some things like data files, generated code, or large data structures you can scan over sometimes, but don’t scan over a human-written class, function, or block of code and assume that what’s inside of it is okay. Obviously some code deserves more careful scrutiny than other code—that’s a judgment call that you have to make—but you should at least be sure that you understand what all the code is doing.
If it’s too hard for you to read the code and this is slowing down the review, then you should let the developer know that and wait for them to clarify it before you try to review it. If you can’t understand the code, it’s very likely that other developers won’t either. So you’re also helping future developers understand this code, when you ask the developer to clarify it.
If you understand the code but you don’t feel qualified to do some part of the review, make sure there is a reviewer on the PR who is qualified, particularly for complex issues such as security, concurrency, accessibility, internationalisation, etc.
It is often helpful to look at the PR in a broad context. Usually the code review tool will only show you a few lines of code around the parts that are being changed. Sometimes you have to look at the whole file to be sure that the change actually makes sense. For example, you might see only four new lines being added, but when you look at the whole file, you see those four lines are in a 50-line method that now really needs to be broken up into smaller methods.
It’s also useful to think about the PR in the context of the system as a whole. Is this PR improving the code health of the system or is it making the whole system more complex, less tested, etc.? Don’t accept PRs that degrade the code health of the system. Most systems become complex through many small changes that add up, so it’s important to prevent even small complexities in new changes.
If you see something nice in the PR, tell the developer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, in terms of mentoring, to tell a developer what they did right than to tell them what they did wrong.
If the there is an issue or just something you would like to see done perhaps differently reject and leave a comment.
If the PR passes all the quality checks simply give it a tick. Once the PR has two ticks it can be merged into the Master branch. Often before that can be done there might need to be a further merge with Master being merged into the PR's branch.
As a team we agreed that if there are over 20 commits then we would squash the commits and merge. This is simply to keep it tidy
If rejected by the PO the PR will need to be discussed and moved from PO Approval back through to development.
The PO should detail exactly why the PR has failed in the Jira ticket. Ticket may be moved back into the Development column or if not as higher priority as your current role into the To Do column for someone else to pick up.
When the PO has given approval of the ticket the PR can be added to the list of other tickets that available to go to Pre-Production. Here the business can look at the PR and give their consent for it to be passed to Production.