在代碼審查/請求請求中尋找什麼?


8

在代碼審查/提取請求期間是否應該尋找任何標準的東西?是否還有針對自動化與應用程序代碼和/或用於UI自動化(使用硒)的特定軟件?

我希望每個項目都有具體的細節,但是在大多數項目中通常都可以期待一個通用的項目嗎?

17

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

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:'[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.


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