7

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?

14

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)

  1. Assertions exist and are more then just true == true
  2. Test description language makes sense and is helpful when the test actually fails
  3. Data. Make sure real personal identification data is not used (PHI/HIPPA/GDPR)
  4. Acronyms and Tribal knowledge are avoided except universal such as DOB, SSN and ID
  5. All examples have test type tag(s) of happy/sad, optional and domain specific as needed
  6. 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)

  1. CSS locators are preferred over xpath whenever possible
  2. Page objects are used for all DOM elements used as selectors
  3. Examples that use data to search use the minimum text needed
  4. Tests mostly rely on framework (e.g. capybara) to handle wait issues
  5. No static value fixed sleeps (any sleeps use values set system wide)
  6. No fixed (explicit) sleeps when polling wait for element / event (implicit) can be used
  7. Page Objects are specific enough to be unique but general enough to be robust (balance)
  8. Complex Data Management structures avoided in favor of simple hard-coded in-line values
  9. Avoid long element locators because they are brittle in that they are tied to the current layout
  10. When possible ID's, classes and container elements are favored over div and span structures
  11. 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
  • 4
    Why do you ask a question to immediately answer it yourself? – FDM Mar 21 '18 at 12:18
  • 4
    @FDM I take advantage of this sqa functionality because I feel I have accumulated a lot of knowledge in my career and I want to share it. When I encounter a situation at work (frequently the genesis for such question-answer pairs) I feel I have a lot of advice that I want to share with both my colleagues and why not the larger community here. So instead of 1 one time email or slack to a few folks I've putting down a list that I hope to be of more use to the general community. I've seen at least one example of each of the above in the last few months so they remain relevant in my experience. – Michael Durrant Mar 21 '18 at 18:39
  • I like your checklist (checklists are good), but I also liked @VishalAggarwal UI automation code is just another piece of code comment, because it summarizes nicely your checklist. I suggest to add it on top of your checklist. – Peter M. - stands for Monica Mar 26 '18 at 18:08
  • 1
    @MichaelDurrant Really appreciate this approach of yours for knowledge and more importantly experience sharing. – Rohan Kalia Jun 30 '18 at 17:35
  • Thanks @RohanKalia that means a lot to me and motivates me to keep adding and improving answers like this. – Michael Durrant Jul 3 '18 at 17:01
8

"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:'test@gl.com')
     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 => ‘test@gmail.com’) 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.

  • 2
    Great answer. I love the quote. – dzieciou Nov 17 at 16:54
2

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:

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).

  • 1
    +! for another great answer @dzieciou – Michael Durrant Nov 17 at 16:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.