What to look for in code reviews / pull requests?

Question

Are there any standard set of things that one should look for during Code Reviews / Pull Requests ? Are there also particular ones for automation vs application code and/or for UI automation (using selenium) ?

I expect every project might have specifics but is there a general set that one can usually expect to look for in most projects?

Answer 1

Here are the top 37 things I check for in Code Reviews

All Code Reviews

1. DRY code
2. Code has tests
3. Linter is being used
4. English readable code
5. Lines are not too complex
6. Typos for spelling and grammar
7. Methods are short (<= 5 lines is ideal)
8. Dependencies are mocked for unit tests
9. Classes are short (< 100 lines if possible)
10. Debug Statements that were not removed
11. Few parameters, usually not more than two
12. Print debug statements that were not removed
13. Lines are not too long, usually < 100-120 chars
14. Security Concerns, e.g. sql_injection in web forms
15. Design patterns are used or followed appropriately
16. Good Names (Classes, methods, variables, constants, etc)
17. Common setup/teardown is extracted to before/after helpers
18. Comments. Try to remove or change to method / object names
19. Code that is obvious to future team members without tribal knowledge
20. Dates. Ensure any date calculations work consistently on all run dates

---

All Automation Code Reviews (plus all above)

21. Assertions exist and are more then just true == true
22. Test description language makes sense and is helpful when the test actually fails
23. Data. Make sure real personal identification data is not used (PHI/HIPPA/GDPR)
24. Acronyms and Tribal knowledge are avoided except universal such as DOB, SSN and ID
25. All examples have test type tag(s) of happy/sad, optional and domain specific as needed
26. Extracted and DRY'd up code generally limits the levels of abstractions to one or two levels

---

UI specific Automation Code Reviews (plus all above)

27. CSS locators are preferred over xpath whenever possible
28. Page objects are used for all DOM elements used as selectors
29. Examples that use data to search use the minimum text needed
30. Tests mostly rely on framework (e.g. capybara) to handle wait issues
31. No static value fixed sleeps (any sleeps use values set system wide)
32. No fixed (explicit) sleeps when polling wait for element / event (implicit) can be used
33. Page Objects are specific enough to be unique but general enough to be robust (balance)
34. Complex Data Management structures avoided in favor of simple hard-coded in-line values
35. Avoid long element locators because they are brittle in that they are tied to the current layout
36. When possible ID's, classes and container elements are favored over div and span structures
37. Ensure 1 smoke, 1 happy, several sad and 0+ optional test type examples exist as appropriate

For pull requests I also look for

• Urgency of change
• What others are saying
• Who else has approved the PR
• If comments have been addressed
• Matching of code to business need
• How long the PR has been out there
• What changes have been made based on feedback

Answer 2

"Programs should be written for people to read, and only incidentally for machines to execute".

-- "Structure and Interpretation of Computer Programs" by Abelson and Sussman

UI automation code is just another piece of "code" hence all the code best practices applies in the same way.

If I have to pick one thing, I would say code shouldn't read like code, it should read like a natural language in the business domain(DSL) on higher layers(test scripts/page object public methods in the automation framework).

All the statements inside a function should be on the same level of abstraction.

Example:

class AddAttendeePage 
  def add_attendee_with_details    
     fill_in('user_email',with:'[email protected]')
     fill_in('user_first_name', with: 'test')     
     fill_in('user_last_name', with: 'test')
     fill_order_form 
     click_add_attendee
  end    
        
  def fill_order_form
     # ...
  end
  def click_add_attendee
     # ...
  end
end

The add_attendee_with_details method here brakes the rule.

The fill_in(‘order_user_email’, :with => ‘[email protected]’) part is more detailed than the fill_order_form part, so the code inside the add_attendee_with_details written on a different level of abstraction.

All the basic 'code' semantics(loops/if/switch) should be deeply buried in the lowest layers of the framework.

If NOT, these are one of most obvious code smells to me that code is not structured properly in different layers in the framework which makes code maintenance a hell due to multiple reasons on multiple levels in the long term.

Answer 3

The area to cover in code reviews deserves a book chapter if not multiple books. First, let's not forget test automation is no different from other branches of software engineering, so let me quote generic suggestions from Gergely Orosz (emphasis mine):

Good code reviews look at the change itself and how it fits into the codebase. They will look through the clarity of the title and description and “why” of the change. They cover the correctness of the code, test coverage, functionality changes, and confirm that they follow the coding guides and best practices. They will point out obvious improvements, such as hard to understand code, unclear names, commented out code, untested code, or unhandled edge cases. They will also note when too many changes are crammed into one review, and suggest keeping code changes single-purposed or breaking the change into more focused parts.

Better code reviews look at the change in the context of the larger system, as well as check that changes are easy to maintain. They might ask questions about the necessity of the change or how it impacts other parts of the system. They look at abstractions introduced and how these fit into the existing software architecture. They note maintainability observations, such as complex logic that could be simplified, improving test structure, removing duplications, and other possible improvements. Engineer Joel Kemp describes great code reviews as a contextual pass following an initial, light pass.

Then there are things that are specific to test automation. Code reviews should specifically point out:

• test automation anti-patterns
• Is inverted test pyramid really anti-pattern?
• What Are Some Examples of Why Explicitly Sleep Statements Are Bad?

I would need to quote whole SQA Stackexchange to make this answer complete.

Finally, let's not forget code reviews play education roles. It's a great way to make new team members aware of code guidelines specific to the team and the project. It's also a great opportunity to teach programming to junior programmers (which often happens in test automation teams).