KSM overcommit test review - 1st pass

[lmr, 2009-12-28 04:02:39]

http://codereview.appspot.com/183068/diff/1/3
File client/tests/kvm/tests/ksm_overcommit.py (right):

http://codereview.appspot.com/183068/diff/1/3#newcode5
client/tests/kvm/tests/ksm_overcommit.py:5: import random, string, math, os
Here we have the import of standard modules after the autotest and custom kvm
ones. All standard module imports should be grouped together.

http://codereview.appspot.com/183068/diff/1/3#newcode19
client/tests/kvm/tests/ksm_overcommit.py:19: def parse_meminfo(rowName):
There are functions on client/bin/base_utils.py that already parse meminfo, we
should consider using them instead of custom functions.

http://codereview.appspot.com/183068/diff/1/3#newcode29
client/tests/kvm/tests/ksm_overcommit.py:29: 
Please don't use a single empty space between functions, as the autotest coding
standard asks for 2:

http://autotest.kernel.org/browser/trunk/CODING_STYLE

I will omit the comment for further occurrences of single line spacing to avoid
cluttering the review, but please fix them all.

http://codereview.appspot.com/183068/diff/1/3#newcode42
client/tests/kvm/tests/ksm_overcommit.py:42: "VM: %s" % vm.name)
Apparently a raise was forgotten on the line above. Also, the best way to do
multi-line statements is using implicit continuation in parenthesis, in general
you should refrain from using \ to continue sentences, ie:

Instead of

print "Fancy multi-line statement"+\
      "that should be avoided."

You should do

print ("Fancy multi-line statement"
       "the preferred way.")

I will omit the comment for further occurrences of ending lines with slash to
avoid cluttering the review, but please fix them all.

http://codereview.appspot.com/183068/diff/1/3#newcode88
client/tests/kvm/tests/ksm_overcommit.py:88: 
Mixing up the body of the main function with declaration of the auxiliary
functions didn't work up well. Please make a clear separation between the body
of the function run_ksm_overcommit and the declaration of auxiliary functions.

http://codereview.appspot.com/183068/diff/1/3#newcode600
client/tests/kvm/tests/ksm_overcommit.py:600: 
Please don't leave up 4 lines of spacing on the code.

http://codereview.appspot.com/183068/diff/1/2
File client/tests/kvm/unattended/allocator.py (right):

http://codereview.appspot.com/183068/diff/1/2#newcode11
client/tests/kvm/unattended/allocator.py:11: 
1) The above imports should be done according to the autotest coding standard,
grouping them into a single line.

2) datetime.timedelta is not being used on the below code, as far as I can see

3) Instead of using datetime.now() use datetime.datetime.now(), I know it's not
exactly pretty but it's the preferred way when writing autotest code.

http://codereview.appspot.com/183068/diff/1/2#newcode13
client/tests/kvm/unattended/allocator.py:13: KVM test definitions.
This summary of the program does not inform what the program is up for. I would
suggest something along the lines "Auxiliary script used for memory allocation
on guests"

http://codereview.appspot.com/183068/diff/1/2#newcode16
client/tests/kvm/unattended/allocator.py:16: Jiri Zupka <jzupka@redhat.com>
Use @author, and your e-mail in parenthesis
@author Jiri Zupka (jzupka@redhat.com)

http://codereview.appspot.com/183068/diff/1/2#newcode205
client/tests/kvm/unattended/allocator.py:205: print "PASS: Start"
Why is that print statement for? It seems out of place

[lmr, 2009-12-28 04:08:43]

Hi Jiri, thank you very much for your work! I have comments to make regarding to
coding style as a first pass review. While reading them, please keep in mind
autotest's coding standards:

http://autotest.kernel.org/browser/trunk/CODING_STYLE

Also, I have noticed lots of trailing spaces on lines. I recommend that every
time you are done working with the code, you use the very useful script
utils/reindent.py (see top level autotest directory) that will remove all
trailing spaces on your code. While I wrote this first pass review, I have
applied it on the resultant files.

[lmr, 2009-12-28 04:10:31]

Here is the full text of the review

http://codereview.appspot.com/183068/diff/1/3
File client/tests/kvm/tests/ksm_overcommit.py (right):

http://codereview.appspot.com/183068/diff/1/3#newcode5
client/tests/kvm/tests/ksm_overcommit.py:5: import random, string, math, os
Here we have the import of standard modules after the autotest and custom kvm
ones. All standard module imports should be grouped together.

http://codereview.appspot.com/183068/diff/1/3#newcode19
client/tests/kvm/tests/ksm_overcommit.py:19: def parse_meminfo(rowName):
There are functions on client/bin/base_utils.py that already parse meminfo, we
should consider using them instead of custom functions.

http://codereview.appspot.com/183068/diff/1/3#newcode29
client/tests/kvm/tests/ksm_overcommit.py:29:
Please don't use a single empty space between functions, as the autotest coding
standard asks for 2:

http://autotest.kernel.org/browser/trunk/CODING_STYLE

I will omit the comment for further occurrences of single line spacing to avoid
cluttering the review, but please fix them all.

http://codereview.appspot.com/183068/diff/1/3#newcode42
client/tests/kvm/tests/ksm_overcommit.py:42: "VM: %s" % vm.name)
Apparently a raise was forgotten on the line above. Also, the best way to do
multi-line statements is using implicit continuation in parenthesis, in general
you should refrain from using \ to continue sentences, ie:

Instead of

print "Fancy multi-line statement"+\
      "that should be avoided."

You should do

print ("Fancy multi-line statement"
       "the preferred way.")

I will omit the comment for further occurrences of ending lines with slash to
avoid cluttering the review, but please fix them all.

http://codereview.appspot.com/183068/diff/1/3#newcode88
client/tests/kvm/tests/ksm_overcommit.py:88:
Mixing up the body of the main function with declaration of the auxiliary
functions didn't work up well. Please make a clear separation between the body
of the function run_ksm_overcommit and the declaration of auxiliary functions.

http://codereview.appspot.com/183068/diff/1/3#newcode600
client/tests/kvm/tests/ksm_overcommit.py:600:
Please don't leave up 4 lines of spacing on the code.

http://codereview.appspot.com/183068/diff/1/2
File client/tests/kvm/unattended/allocator.py (right):

http://codereview.appspot.com/183068/diff/1/2#newcode11
client/tests/kvm/unattended/allocator.py:11:
1) The above imports should be done according to the autotest coding standard,
grouping them into a single line.

2) datetime.timedelta is not being used on the below code, as far as I can see

3) Instead of using datetime.now() use datetime.datetime.now(), I know it's not
exactly pretty but it's the preferred way when writing autotest code.

http://codereview.appspot.com/183068/diff/1/2#newcode13
client/tests/kvm/unattended/allocator.py:13: KVM test definitions.
This summary of the program does not inform what the program is up for. I would
suggest something along the lines "Auxiliary script used for memory allocation
on guests"

http://codereview.appspot.com/183068/diff/1/2#newcode16
client/tests/kvm/unattended/allocator.py:16: Jiri Zupka <jzupka@redhat.com>
Use @author, and your e-mail in parenthesis
@author Jiri Zupka (jzupka@redhat.com)

http://codereview.appspot.com/183068/diff/1/2#newcode205
client/tests/kvm/unattended/allocator.py:205: print "PASS: Start"
Why is that print statement for? It seems out of place

On Mon, Dec 28, 2009 at 2:08 AM,  <lookkas@gmail.com> wrote:
> Hi Jiri, thank you very much for your work! I have comments to make
> regarding to coding style as a first pass review. While reading them,
> please keep in mind autotest's coding standards:
>
> http://autotest.kernel.org/browser/trunk/CODING_STYLE
>
> Also, I have noticed lots of trailing spaces on lines. I recommend that
> every time you are done working with the code, you use the very useful
> script utils/reindent.py (see top level autotest directory) that will
> remove all trailing spaces on your code. While I wrote this first pass
> review, I have applied it on the resultant files.
>
>
> http://codereview.appspot.com/183068
>



-- 
Lucas