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?

up vote 11 down vote accepted

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

All Code Reviews

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

All Automation Code Reviews (plus all above)

  1. Test description language makes sense and is helpful when the test actually fails
  2. Data. Make sure no consumer data is used and no references to company names exist
  3. Acronyms and Tribal knowledge are avoided except universal such as DOB, SSN and ID
  4. All examples (or parents) have tag(s). Happy/sad, optional and domain specific as needed
  5. 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. When possible ID's, classes and container elements are favored over div and span structures
  10. Assertions exist and, except smokes, are more then just expect(find('page.body')).to be
  11. Ensure 1 smoke, 1 happy, several sad and 0+ optional test type examples exist as appropriate
  12. Avoid long element locators because they are brittle in that they are tied to the specific layout structure which will tend to change over time. If elements do not have good identifiers and you end up trying to use page objects that are very brittle e.g. (real world example): add_bank_account_confirm: '#main_content_container > div:nth-child(3) > div.multicolumn_column.width_33 > div > div.article.article_standard > p:nth-child(32) > a > span > strong' then switch to another approach such as click_link('Add bank account')

For pull requests I look for

  • What others are saying
  • Who else has approved the PR
  • If comments have been addressed
  • How long the PR has been out there
  • What changes have been made based on feedback
  • 3
    Why do you ask a question to immediately answer it yourself? – FDM Mar 21 at 12:18
  • 3
    @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 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. – P.M Mar 26 at 18:08
  • 1
    @MichaelDurrant Really appreciate this approach of yours for knowledge and more importantly experience sharing. – Rohan Kalia Jun 30 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 at 17:01

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

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

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

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

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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