[Plone-testing-team] Code Analysis / Git pre-commit hook

[Timo Stollenwerk]

Am 15.05.2013 02:29, schrieb Héctor Velarde:
> sorry for not taking care of this before, but I was very busy managing a
> couple of projects over here and making the new release of
> collective.cover.
>
> I will love to start some discussion on what and how are we going to
> test with this QA stuff. I think there are 2 different places we want to
> use it: Plone core and Plone add-ons.
>
> I know you, and many others, have been working on the core, and I,
> toutpt and many others, have been working on the add-ons.
>
> so I'm going to list some questions that I think we must answer first,
> and I'm going to include my own answers:
>
> 1. where are we going to put this?
>
> https://github.com/collective/buildout.plonetest/blob/master/qa.cfg
> seems to be a good place for me, at least for the add-ons... what about
> core packages?

I started to refactor my code into a buildout recipe. A recipe can 
provide a well defined API (because we can change the API between 
versions) and can be used in different contexts.

> 2. what are we going to test?
>
> AFAIK people is currently testing the following:
>
> - code validation: we are using pep8/pyflakes or flake8, the later is my
> personal favorite and includes also a McCabe complexity checker.
>
> http://pypi.python.org/pypi/flake8

+1

I'm using flake8 as well.

> - test coverage: I'm using createzopecoverage and a script I wrote for
> that; we can get rid easily of the script if we are able to make
> createzopecoverage return the current test coverage as an exit code.

I had better experience with coverage because it seems to be better 
documented and maintained. We should also make sure that jenkins and 
code analysis use the same tools.

> http://pypi.python.org/pypi/createzopecoverage
> https://github.com/hvelarde/qa/blob/master/templates/coverage.sh.in
>
> - CSS validation: I think we agree on csslint but none of us have had
> the time to dig into its options; we need to start probably enforcing
> only well-formed CSS and then we can add some other options and make it
> user configurable.
>
> https://github.com/stubbornella/csslint/wiki/Rules

By default CSSLint complains about way too many things. If we can 
configure it that would be great.

> - JS validation: same here with jshint, we must start enforcing only
> well-formed JS and make it user configurable.
>
> http://www.jshint.com/docs/#options
>
> note that both, csslint and jshint, need Node.js and npm installed to
> run (in Travis VM they are installed by default).

My pre-commit hook expects jshint and csslint to be installed on the 
system, otherwise it will not test js/css. Using buildout recipes to 
build node did not really work reliably.

> - ZPT validation: I saw you're using zptlint, but I don't know how good
> it is; we should probably ask gotcha to move it to GitHub so we can
> improve it.

+1

> - ZCML validation: I saw some people using zptlint for this but I have
> no idea if it works at all.

I haven't looked into that yet.

> - XML validation: same as the above.
>
> - UTF-8 headers: that's a good and easy one that you already have; we
> were bitten by this the other day and most people seem to have forgotten
> what an UTF-8 BOM is.

+1

> - pdb and console.log inclusion: very easy also and a must have.

+1

> - dependencies validation: I manually use z3c.dependencychecker but I
> think we must developed an automatic way to check this... as soon as we
> agree on what and how to do it.
>
> https://pypi.python.org/pypi/z3c.dependencychecker

+1

> - i18n validation: based on Maurits' script we tried to add some other
> options to give more functionality, but I'm not pretty convinced on the
> results:
>
> https://github.com/hvelarde/qa/blob/master/templates/rebuild_i18n.sh.in
>
> i18ndude find-untranslated is a must have here.
>
> - documentation validation: I personally have no idea on this yet
> because I never have written documentation for our packages. I think
> plone.api and collective.developermanual are good starting points.

ReST checkers would be nice.

> - package distribution: what about checking for things like package
> classifiers, MANIFEST.in, licenses...?
>
> - accessibility validation: polyester told me they were doing something
> like that but I have no idea on how to accomplish it.
>
> 3. how are we going to test and configure? I saw you're using only
> scripts; I took the idea of the Makefile from zupo, and toutpt brought
> the templates.

As said above. I think we should create a buildout recipe because it is 
the most flexible option.

> I prefer to use separate templates instead of inline because it's easier
> to edit and maintain a well documented template this way.
>
> I really love all the possibilities open by the use of the make command
> but I think few of us really know how to use them.
>
> the Makefile makes validation and testing cleaner and run things in
> different order it's also easier.

I'm not a big fan of using Makefiles. We should use buildout because two 
ways of doing things confuses people. I don't see big advantages of 
Makefiles.

> the use of configuration variables is quite important here; toutpt
> included some on buildout.plonetest but I don't agree with all, neither
> with their names; this is my personal proposal:
>
> package-pep8-ignore
> package-max-complexity
> package-csslint-ignore
> package-jshint-ignore
> package-minimum-test-coverage

Are these the names for the bin files? I used code-analysis-pep8 etc. so 
far.

> I think names are descriptive and I also think we don't need
> 'package-src' at all, because we can guess this at run time.
>
> 4. when are going to enforce QA? some people are more picky that others:
> I thinks test must fail if any validation fails (except for the E501 on
> PEP 8), but I think we must leave this to the owner of the package; we
> need an easy way to configure which validations are critical and which
> ones are optional or desirable.

Our solution should be as flexible as possible. We need strict rules for 
coredev though.

> we probably need to add a variable to list the validations that must be
> enforced.
>
> and we may want to have colorful outputs to make easier to find errors
> and warnings; I was able to do so by using bash as the preferred shell
> and some ANSI escape sequences (see the Makefile).

My current scripts already have colored outputs. My main question right 
now is, do we always want to test the entire package code for each 
commit or just the files that have been changed in the commit. I used 
the latter a bit and it confused me, so I tend to prefer the 
always-test-everything approach.

Unfortunately I don't have much spare time right now but I will try to 
upload my work to github so people can have a look.

Cheers,
Timo

-- 
Timo Stollenwerk
web application developer

Bornheimer Straße 37
53111 Bonn

office +49 228 - 286 288 380
fax    +49 228 - 286 288 389

timostollenwerk.net
contact at timostollenwerk.net