-
Notifications
You must be signed in to change notification settings - Fork 0
Code Review (PR) process
We perform a code review in order to ensure that the produced code meets the Quality Assurance Surveillance Plan, as defined in the RFQ:
| Deliverable | Performance Standard(s) | Acceptable Quality Level | Method of Assessment |
|---|---|---|---|
| Tested Code | Code delivered under the order must have substantial test code coverage | Minimum of 90% test coverage of all code. All areas of code are meaningfully tested | Combination of manual review and automated testing |
| Properly Styled Code | 18F Coding Styles Reference Guide | 0 linting errors and 0 warnings | Combination of manual review and automated testing |
| Accessible | Web Content Accessibility Guidelines 2.1 AA standards; Section 508 Compliance | 0 errors reported using an automated scanner and 0 errors reported in manual testing | Combination of manual review and automated testing (such as pa11y) |
| Deployed | Code must successfully build and deploy into staging environment | Successful build with a single command | Combination of manual review and automated testing |
| Documented | Summary of user stories completed every sprint. All dependencies are listed and the licenses are documented. Major functionality in the software/source code is documented. Individual methods are documented inline in a format that permits the use of tools such as JSDoc. System diagram is provided. Relevant security controls are documented and kept up to date. | Combination of manual review and automated testing, if available | Manual review |
| Secure | Code is free of known static and runtime vulnerabilities | Code submitted must be free of medium- and high-level static and dynamic security vulnerabilities | Tests free of medium- and high-level vulnerabilities from a static testing SaaS (such as Snyk or npm audit), from dynamic testing tools like OWASP ZAP (with documentation explaining any false positives), and ongoing code review informed by OWASP or similar standards |
| User research | Features and functionality developed should be driven by user insights and data analytics. Usability testing and other user research methods must be conducted at regular intervals throughout the development process (not just at the beginning or end). | Research plans and artifacts from usability testing and/or other research methods with end users are available at the end of every applicable sprint, in accordance with the Contractor’s research plan. | SmartPay will manually evaluate the artifacts based on a research plan provided by the contractor at the end of the second sprint and every applicable sprint thereafter. |
It is important to approach a code review with the right frame of mind:
- Expect to ask lots of questions, not as an interrogation, but as an ongoing conversation, taking place between the government and the vendor.
- This is an Agile process and, as such, we treat the output of every sprint as a completed product. Security, accessibility, UX, tests, browser compatibility, etc. are not features to be implemented in future sprints, but are things that we expect to be incorporated constantly, and completed at the end of each sprint.
- Remember that we are all learning together here. If you see something in the code that you don’t understand, it’s incumbent on you to say so. Maybe you know something that the developer doesn’t, maybe the developer knows something that you don’t, but the only way to find out is to ask.
Code reviews are about keeping code clean and limiting technical debt. We will look for things that increase technical debt or create an environment where technical debt can be introduced easily later.
We’re mostly going to look at new and changed code. For changed code, we’ll check the existing tests to make sure they’ve been updated if necessary. For new code, we’ll check that new tests were created.
We look to ensure that new and/or changed code does not contain secrets (account names, passwords, private keys, private hostnames, service endpoints and other confidential secrets).
Tests should cover all branches of logical decisions (e.g., if / else statements). We’ll check this by looking at a code coverage report that shows which lines were executed.
Boolean operations can be somewhat-hidden logical paths. That is, a code coverage report will show every line is tested, but if boolean parts of a condition aren’t fully exercised, then some logical paths aren’t actually tested. We’ll check the tests to make sure they check both sides of logical operations.
Test names shouldn’t be overly technical. Ideally folks outside the development team can read the tests and know what’s passing and what’s failing. Test names should describe their behavior and not just be the name of the method being tested. We’ll check the test names to make sure they’re descriptive.
It’s easy to get to 100% code coverage without actually testing anything. We’ll look at the tests themselves to make sure they’re actually making the right assertions about the methods under test.
We’ll look for duplication in the code or excessively complex code where it might make sense to break functionality into methods that can be simplified or reused.
Methods that are hard to reason through are also difficult to test, difficult to maintain, and prone to bugs. We’ll look out for methods that are complex, or unusually long, and suggest either refactoring that method or possibly breaking it into smaller pieces.
Method names should accurately reflect what the method does, and variable names should pretty clearly indicate what data they’re holding. Don’t be afraid of long names. This also applies to method argument names. Ideally, someone looking at a method signature should be able to infer what it does without any additional documentation. We’ll look at these names to be sure they make sense. Good naming practices contribute to self-documenting code and reduce the manual documentation burden.
With good version control, it should be unnecessary to comment-out blocks of code — just delete them and get them from source history if you really need them again in the future. Obviously it’s fine to comment out code while you’re developing, but once a feature is ready to merge, that former code should just be removed. We’ll be looking for these commented code blocks.
Comments in the code should describe complex bits of logic that aren’t easily glanceable — if someone new to the code can’t skim it and understand it, a comment might be in order. As we’re reviewing the code, if we find a bit we can’t understand quickly from the code and context, we’ll be looking for a comment that explains it. Comments should appear with the code they’re describing.
If code exposes a public API — whether that’s public methods on a class or HTTP endpoints in a REST service — those public methods should be documented. We like documentation that can be extracted into some pretty markup (e.g., .NET’s XML comments, OAS, Swagger). We’ll check that any public-facing methods have useful documentation.
The project should adopt a code style guide and code should conform. Which guide the team chooses is less important than the consistency that comes from actually using it. We’ll check to make sure there’s a linter configured to check code style, that it passes, and that any exceptions are documented and explained in the code.
Code is a UX. Its users are other developers. In the spirit of user-centered design, we’ll be interested in the experience of future developers. We’ll look at code with this question in mind: “Would I want to work on this?”
After the work to write the code, it deserves for us to give it proper attention. We’ll take the time to carefully look over it and give our feedback.
If we’re not sure about something, we’ll ask for clarity. We may ask a lot.
We wouldn’t want anyone being mean to us because of an oversight, mistake, or just a different idea, and we’ll extend that courtesy to others. Code reviews are really conversations, not dictates. We’ll make suggestions rather than simply saying, “that’s wrong.”
We use this list when performing a code review to ensure that all tasks have been completed.
- Review the pull request itself, to get oriented
- Read the description of the pull request, which should summarize the changes made
- Read through every task on the project board that's encompassed by this pull request
- Read the description of the commits that comprise the pull request
- If changes were made in the UI (if not, skip):
- Inspect the changes on the preview branch on cloud.gov Pages
- Test all functionality in all major browsers, emphasizing the functionality that this pull request addresses
- Use an automated audit tool for code quality and practices (recommended: Chrome DevTools, aka Lighthouse)
- Look at efficiency of page loads, asset sizes, HTTP connection management, etc.
- Review for accessibility
- Use an automated audit tool, such as Chrome Audit or aXe
- Navigate site only with the keyboard
- Use VoiceOver or Narrator to navigate the site with audio only, with the display turned off
- Manually test anything that pa11y cannot test automatically (e.g., contrast of text over images)
- Review static code analysis results in GitHub CodeQL logs and DependaBot
- Examine output from dynamic testing tools, to ensure that any errors are known to be false positives or have been previously declared to be acceptable
- Skim all new code, in the context of existing code, looking for problems (knowing that the vast majority of new code will be covered by tests)
- Review all tests
- Look at code coverage of tests in GitHub Actions
- Methodically review all new tests for correctness, quality of naming
- Determine what code isn’t tested, review that rigorously
- Review documentation to ensure that it matches changes
- Provide comments on the pull request on GitHub, as necessary
- For comments that are specific to a particular line of code, comment on those specific lines
- For each feature-level bug (i.e., it’s working as designed, but designed wrong), open a new issue and put it in the backlog