Skip to content

Commit

Permalink
Update docs
Browse files Browse the repository at this point in the history 
1. README.rst:
 a. link review summary to give it more visibility
 b. some formatting to improve readability

2. review summary:
 a. rst formatted to improve readability
 b. added note about escape sequences and variable declaration location
  • Loading branch information
@smitterl
smitterl committed Nov 7, 2019
1 parent 7259203 commit 080ff92
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 77 deletions.
40 changes: 19 additions & 21 deletions README.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
Libvirt test provider for virt-test
===================================

This is the official [1] test provider for the following
This is the official test provider [1]_ for the following
subtest types:

* Libvirt
Expand All @@ -13,15 +13,14 @@ subtest types:
Really quick start guide
------------------------

1) Fork this repo on github
2) Create a new topic branch for your work
3) Create a new test provider file in your virt test repo,
like:

::
1. Fork this repo on github
2. Create a new topic branch for your work
3. Create a new test provider file in your virt test repo,
like::

cp io-github-autotest-libvirt.ini myprovider.ini
::

::

[provider]
uri: file:///home/foo/Code/tp-libvirt
Expand All @@ -33,21 +32,20 @@ Really quick start guide
subdir: lvsb/
[v2v]
subdir: v2v/
You can optionally delete temporarily the
`io-github-autotest-qemu.ini` file, just so you don't have test
conflicts. Then you can develop your new test code, run it
using virt test, and commit your changes.

4) Make sure you have `inspektor installed. <https://github.com/autotest/inspektor#inspektor>`_
5) Run:
You can optionally delete temporarily the
`io-github-autotest-qemu.ini` file, just so you don't have test
conflicts. Then you can develop your new test code, run it
using virt test, and commit your changes.

::
4. Make sure you have `inspektor installed. <https://github.com/autotest/inspektor#inspektor>`_
5. Run::

inspekt checkall --disable-style E501,E265,W601,E402,E722,E741 --no-license-check
inspekt checkall --disable-style E501,E265,W601,E402,E722,E741 --no-license-check <test_script_name>.py

6) Ensure check-list in tp-libvirt_review_comment_summary.rst are met
7) Fix any problems
8) Push your changes and submit a pull request
9) That's it.
6. Ensure <https://github.com/autotest/tp-libvirt/blob/master/tp-libvirt_review_comment_summary.rst> are met
7. Fix any problems
8. Push your changes and submit a pull request
9. That's it.

[1] You can always create your own test provider, if you have special purposes, or just want to develop your work independently.
.. [1] You can always create your own test provider, if you have special purposes, or just want to develop your work independently.
154 changes: 98 additions & 56 deletions tp-libvirt_review_comment_summary.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,68 +2,110 @@
Review comment summary for tp-libvirt
======================================

Overview:
======================================================================
Overview
======================================================================

This doc summarize the most common review comments in tp-libvirt open source project.

This doc summarize the most common review comments in tp-libvirt open source project.
For beginners or practice executor, it may provides some self-check before submitting to normal review in github.
For beginners or practice executor, it may provide some self-check before submitting to normal review in github.

======================================================================
Goal
======================================================================
Goal:

1)Provide one place to accumulate knowledge
2)Provide some helps to beginners or executors in coding or style format for tp-libvirt
1. Provide one place to accumulate knowledge.
2. Provide some help to beginners or executors in coding or style format for tp-libvirt.

======================================================================
Common issues
======================================================================

More common issues during reviewing can be categorized as below.
Format::
--cfg file
trailing whitespaces
indent alignment
variable reusable
variable name(upper or lower case letter)
Module import order(better similar module stay together)
Spelling errors
--py file:
imported module one blank line
miss doc comments
doc comment need one empty line between comment and parameter
doc comment start with #<space>'
comments include multiple lines, need care ','
indent issue
trailing whitespace
unused variable
remove comment code(#...)
comment upper and lower letter issues
logging.debug("Find snapshots: %s", snap_names)
Spelling errors

======================================================================
Coding:
-- variable name issue:
name should be meaningful
-- depreciated method:
test.skip(remove raise)
autotest.client import utils
-- result assert
libvirt_vm.check_exit_status
-- resource cleanup
vm_connection_session close(exception gracefully close)
vm_backup.sync() called before any change
-- variable not definition
extreme situation: variable not definition such as conditional
-- parameters in method is not usable
Use autoflake to remove unused parameters
-- list index out of range
make sure that your list index
-- duplicate code avoidance
when code was called twice, better package them into one method
-- exception handling
miss do assert in throwing exception
-------------
Format
-------------
- cfg file

- trailing whitespaces
- indent alignment
- variable reusable
- variable name (upper or lower case letter)
- module import order (better similar module stay together)
- spelling errors

- py file:

- imported module one blank line
- missing doc comments
- doc comments need one empty line between comment and parameter
- doc comments start with #<space>'
- comments include multiple lines, need care ','
- indent issue
- trailing whitespace
- unused variable
- remove comment code (#...)
- comment upper and lower letter issues
- logging.debug("Find snapshots: %s", snap_names)
- spelling errors
- escape sequences, should prepend regex patterns with 'r'

-----------------
Coding
-----------------
- variable name issue:

- name should be meaningful

- variable declaration:

- should be defined on the very top (avoids local redefinition and undefined names during tear down in case of error)

- deprecated method:

- test.skip (remove raise)

- autotest.client import utils

- result assert

- libvirt_vm.check_exit_status

- resource cleanup

- vm_connection_session close(exception gracefully close)

- vm_backup.sync() called before any change

- variable not definition

- extreme situation: variable not definition such as conditional

- parameters in method is not usable

- Use autoflake to remove unused parameters

- list index out of range

- make sure that your list index

- duplicate code avoidance

- when code was called twice, better package them into one method

- exception handling

- miss do assert in throwing exception

======================================================================
Enhancement(Best practice):
-- conjunction two folders:use os.path.join(xx,xx)
-- avoid too generic logging message
-- not recommended to use mutable default value as an argument see: https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html
-- without timeout value in infinite loop
-- use with to open files
-- make sure this is run before sending patch:inspekt checkall --no-license-check <*>.py
Enhancement (best practice):
======================================================================
- conjunction two folders:use os.path.join(xx,xx)
- avoid too generic logging message
- not recommended to use mutable default value as an argument see <https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html>
- without timeout value in infinite loop
- use with to open files
- make sure this is run before sending patch::

inspekt checkall --disable-style E501,E265,W601,E402,E722,E741 --no-license-check <test-script-name>.py

0 comments on commit 080ff92

Please sign in to comment.