oca / maintainer-quality-tools Goto Github PK

Acknowledging the different repositories (and branches) might be in different quality maturity levels, it has been decided to implement "releases" for the Quality Tools. (See #84 for details).

Each repository branch can pick the release to use, according to it's specific maturity and needs.Currently we have three available releases:

• r1: dates from 23/July, and is the last version allowing lines up to 120 characters.
• r2: dates from today (9/Sep), has lines limited to 79 chars, has better coverage measurements, and supports INCLUDE and EXCLUDE directives.
• master: is where the most recent additions to quality checks will be merged, such as better linting.

A "release" is just a branch of the OCA/maintainer-quality-tools, so they can evolve, but mostly to include fixes. Currently all repositories are using master. To pick a specific release, just add -b option to the git clone in the .travis.yml. For example:

git clone https://github.com/OCA/maintainer-quality-tools.git $HOME/maintainer-quality-tools -b r2

We will keep on hold for a few days the pending pull requests, to allow changes to be done on the relevant projects.

Whenever we feel additional checks could be included, that could have significant impact on existing repositories, we will consider a release 3 "stability point" and announce upcoming changes it in advance.

Any questions or suggestions for this process, this is also the place to ask and discuss.
Even if not interested in watching this repository discussions, it is advised to subscribe this Issue to learn about any changes in releases.

Could someone remind me why we do this? From my understanding of https://github.com/odoo/odoo/blob/master/openerp/netsvc.py#L192, we end up with log_level=[] which is the same as info, I think?

It is different in 8.0 because Odoo will actually return a proper return code, so tee should not be used.

I am looking at this line

- travis_run_tests 7.0 $HOME/connector $HOME/connector-ecommerce $HOME/sale-workflow $HOME/e-commerce $HOME/product-attribute

It seems this ($HOME/connector $HOME/connector-ecommerce $HOME/sale-workflow $HOME/e-commerce $HOME/product-attribute ) could be automatically found. It is redundant as the dirs are already stated in the install section, where they are cloned.

If we agreed on a directory, say (/tmp/repo-dependencies). The travis run tests script could look in that dir and automatically add those to the addons paths.

That would make adding a repo dependency easier, the line travis_run_tests 7.0 wouldn't need to explicitely list what has already been listed in the install section.

I proposal active next error from pylint conf

E1101 - no-member
When you use a method not defined of a object.
For example

VAR = []
VAR.strip()

pylint message: [E1101(no-member), ] Instance of 'list' has no 'strip' member

W0403 - relative-imports
More info here
To check a correct import

from . import wizard

W0622 - Redefining built-in
if you have

file = 'other thing'

pylint message: [W0622(redefined-builtin), ] Redefining built-in 'file'
Because file is a main method from internal python

W0621 - redefined-outer-name

import os

def method1(os):
    print os

pylint message: [W0621(redefined-outer-name), method1] Redefining name 'os' from outer scope (line 5)

W1401 - Anomalous-backslash-in-string
String constant might be missing an r prefix. Used when a backslash is in a literal string but not as an escape.

regex_test = '\d'

[W1401(anomalous-backslash-in-string), ] Anomalous backslash in string: '\d'. String constant might be missing an r prefix.

This will fix it with:

regex_test = r'\d'

W0404 - reimported

import time
import os
import time

pylint message: [W0404(reimported), ] Reimport 'time']

W0104 - pointless-statement

number=3
number/3

pylint message: [W0104(pointless-statement), ] Statement seems to have no effect

W0105 - pointless-string-statement

'''
docstring cool
'''

def method():
    '''docstring cool'''
    print "hello world"
    '''
    This not is a docstring and this is a pointless-string-statement
    '''

pylint message: [W0105(pointless-string-statement), method] String statement has no effect

W0109 - duplicate-key

dict_example = {
    'key1': 'value1',
    'key1': 'value2'
}

[W0109(duplicate-key), ] Duplicate key 'key1' in dictionary

    env:
     - PATH=$HOME/maintainer-quality-tools/travis:$PATH

This is not what env should be used for.

You can export PATH in the before_install section.

env should be used to set up different build environments, the same was as it is with the python section.
ex:

env:
    - ODOO=https://github.com/odoo/odoo`
    - ODOO=https://github.com/OCA/OCB`

To run the build twice, once with odoo, once with OCB.

You then use $ODOO in the install and/or before_install sections to have the builds be different in their respective environments.

We should fail on deprecation lines such as:

2014-10-29 14:32:46,506 23871 WARNING openerp_test openerp.addons.base.ir.ir_ui_view: Setting the `type` field is deprecated in the `ir.ui.view` model.

In https://github.com/OCA/maintainer-quality-tools/blob/master/travis/travis_run_tests#L76, we grep for 'mail' before looking for anything else. Why would we do that? On my system, this causes no failures to be caught by this line.

The current use of coveralls used in PR #29 is meaningless for shell scripts.
Would it be better to remove it?
Or perhaps add shell test coverage checking using shcov or similar.

It would be great to have an official sample for the README.md file, especially the badges parts, but this could be extended in the future to get more useful boilerplate (link to OCA website...)

In .travis.yml:

env:
  - odoo=Official
  - odoo=OCB

Will run two different travis tests one with $odoo having the value 'Official' and one being 'OCB'

This will allow us to catch errors which exist on one platform and not the other.

Add unit test capability, where each module would be individually installed and tested.
This would allow to detect implicit dependencies.
Setup time can be reduced by installing a base Odoo database, and then use it as a template for the database used my each modules test.
If done through a third "environment" (alongside with odoo/odoo and oca/ocb), it would be run in parallel, making test builds finish faster.

I'm working on the configuration of the runbot for OCA project, and this requires setting up the dependencies of the various projects, a process which is quite tedious, and also related to the writing of .travis.yml where such dependencies need to be manually coded.

I was thinking of a metadata file at the project level describing the inter-repository dependencies which could be used to help in the process. In the simplest form, it would be a plain text file, with 1 repository per line giving the names of the repository on which the current project depends. The scripts launched by travis could be updated so that in case the file is present, they would checkout the listed repository with the ${VERSION} branch. And having this information available would allow for an easier configuration of the runbot (at the moment, I'm grepping for git clone lines in .travis.yml...)

If people agree with this idea, I'll propose an implementation.

I have disabled the email sent every friday to encourage people to review merge requests on Launchpad, based on the nag script from lp:lp-community-utils

If someone can help me update the script to use Github's pull requests, I will enable the email.

Thanks!

Hi,

We're trying to generate the instance using travis in a Docker container, so fa we've succeded launching it alone and running pylint, but when travis tries to execute the tests (or after) the following error is shown:

Screenshot:
http://www.zimagez.com/zimage/capturadepantalla-290914-134904.php

Error:
UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-31: ordinal not in range(128)

As far as I can see the error raises from this line:
https://github.com/OCA/maintainer-quality-tools/blob/master/travis/travis_run_tests#L25

Here we have the output of travis:

The command "travis_run_tests 7.0 openerp_test" exited with 0.
after_success
$ coveralls
Submitting coverage to coveralls.io...
No source for memory:0x2a5efd0
No source for memory:0x2bf3690
No source for memory:0x2bf3d10
No source for memory:0x3030210
Coverage submitted!
Couldn't find a repository matching this job.
'url'
Done. Your build exited with 0.

The test output are not shown even it with the tee comad

The output or coverall seems also odd...

Thanks

As seen here (https://github.com/OCA/carrier-delivery/pull/19/files#r15347475), we are discussing maximum line length to check by flake8.

(with votes from mailing list: http://openerp-community.2306076.n4.nabble.com/Openerp-community-Coding-Guidlines-long-lines-td4643741.html)

Hello,
I want to know Why we use a wget to odoo.tar.gz
This generates a bit of complexity in the algorithms of fails.
*Split "/" to get repo and later version in each script that needs it.
https://github.com/OCA/maintainer-quality-tools/blob/master/travis/test_pylint#L18
https://github.com/OCA/maintainer-quality-tools/blob/master/travis/test_server#L125
*Process to unzip odoo.tar.gz.
https://github.com/OCA/maintainer-quality-tools/blob/master/travis/travis_install_nightly#L15

We can simplify this with

git clone --depth 1 -b BRANCH ${HOME}/STANDARD_FOLDER

And use it directly in other scripts.

NOTE: depth 1 is to get last version without history

There is no hook to allows to run some command after database creation or at least to deactivate database creation:

For geospatial addons I need to run:

createdb openerp_test
psql -U postgres -c "create extension postgis" openerp_test

In the past few days I've been noticing flake8 complaining about errors I'm sure were no there before.
To be more specific:

E123 closing bracket does not match indentation of opening bracket's line

(example: https://travis-ci.org/OCA/server-tools/jobs/38358886)

Was the any change in flake8 configuration or defaults I didn't notice?
My local flake8 doesn't report these errors.

This is happening on https://github.com/OCA/server-tools/tree/8.0/base_field_serialized and was found here: https://travis-ci.org/OCA/server-tools/jobs/36851836#L520

The problem is the rule that checks if there are any security access rules missing on new models.
These type of creates create new modules just for testing purposes, but it's it raise the warning on the log, and the current TravisCI checks then report a failure.

How to solve this? Adopt the convention to ignore errors for lines containing test_model or test.model?

Many times our developers add "print" or "pdb" sentence to traceback a code.
However, it is susceptible to forget to remove when doing commit and push
With ast is easy check this sentence.
See example here:
https://github.com/Vauxoo/pylint-conf/blob/master/check_ast/check_print_and_pdb.py

I can make a PR to add "custom_lint" script.

How do you think?

See
https://travis-ci.org/OCA/l10n-canada/builds/30682141

Point of info, I created the 8.0 branch on OCB yesterday from master

See https://travis-ci.org/OCA/l10n-italy/builds/32227153

2014-08-11 13:13:17,804 23279 INFO openerp_test openerp.modules.loading: Modules loaded.
--------
Traceback (most recent call last):
File "/home/travis/maintainer-quality-tools/travis/travis_run_tests", line 105, in <module>
exit(main())
File "/home/travis/maintainer-quality-tools/travis/travis_run_tests", line 97, in main
errors = has_test_errors("stdout.log", database)
File "/home/travis/maintainer-quality-tools/travis/travis_run_tests", line 22, in has_test_errors
stdout = subprocess.check_output(cmd, shell=True).split('\n')
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 6094: ordinal not in range(128)
The command "travis_run_tests 7.0 $HOME/account-invoicing $HOME/account-financial-tools $HOME/webkit-tools $HOME/account-payment" exited with 1.

Is this related to #61?
@dreispt any hint?
Thanks

This is quickly getting complex. I'm starting to feel the need to test the tests ;-) but not sure on how to do that.

Related to OCA/account-financial-tools#16

What needs to be done:

• no triggering of flake8 false positives on oerpscenario step definition code
• run the oerpscenario tests of the addons, if any

There is an issue with this fix, though : say a repo contains 2 modules mod1 and mod2, with mod2 depending on mod1, and mod2 defines a new required field on a model defined in mod1.

Then, it will often be the case that mod1's tests will FAIL if mod2 is installed (because a new record cannot be created). A typical example of this in the official addons is sale and delivery : the tests for sale fail if delivery is installed.

This is why I had the installation of base only in a first step, and then the installation of the tested modules with tests enabled which would have the side effect of also testing the dependencies of the addons.

A better fix for issue #22 is to find out the dependencies of the addons in the repo which are not in the repo, and install only these first, with tests disabled, and then to init the addons of the repo with tests enabled.

Hi

I found an issue with travis_run_test that doesn't detect correctly an error on the log output and return an exit code 0

my error provide when i declare my wizard file in the openerp.py after the view file that containt the button whom call the action

see result: https://travis-ci.org/mga-team-odoo/jasperserver/jobs/31813285

Regards,

With the merge of the new pylint checks (1c61925), correct branches goes red. See for example this travis log:

https://travis-ci.org/odoomrp/odoomrp-wip/jobs/38802829

Even a supposed ignore condition, shadow id variable name, is checked.

We must revert it ASAP and study later more carefully.

pylint allow check class named and method named with regex.
I don't know if is a good idea make a PR enabling this check to enforce standard.

What do you think?

From http://docs.travis-ci.com/user/multi-os/:

This feature needs to be enabled manually. If you wish to take advantage of this feature, please send email to [email protected], indicating which repository you would like to have it enabled.

Setting .travis.yml

In the repository for which the multi-OS feature is enabled, add the following:

os:
  - linux
  - osx

TRAVIS_OS_NAME environment variable

To distinguish the jobs, you can use $TRAVIS_OS_NAME. The value corresponds to the os value in .travis.yml, either linux or osx.

Hello,

Doing a grep on At least one test failed when loading the modules is maybe not a good idea because if there is a missing depency or a fatal error in tests travis run will be green

We are add new test
And now we need change our .travis in each project to get this test.

If we use a global run_all_test script for add all test by default.
And our .travis in each project set run_all_test
This is more easy of use

What do you think?

Here: https://github.com/OCA/maintainer-quality-tools/blob/master/travis/cfg/travis_run_flake8.cfg#L2
and there: https://github.com/OCA/maintainer-quality-tools/blob/master/travis/cfg/travis_run_flake8__init__.cfg#L3

The pep8 limit should be 79 characters. The nominal length is 80 (79 chars + \n), but the checker should be configured to 79. Although the default option for flake8 is 79:

flake8 --help
Usage: flake8 [options] input ...

Options:
  --version             show program's version number and exit
[...]
  --max-line-length=n   set maximum allowed line length (default: 79)
[...]

Using the flake8 test, we could do more checks such as:

• is there a LICENSE file
• pylint score
• xmllint score
• jshint (javascript linter, in package node)
• dtd verification of data and view xmls
• are there #vim lines.
• is encoding specified
• only valid __openerp__.py keys used
  • Odoo conventions
  • See if @nbessi is ok with us adopting openerp-conventions
• Is deprecated python used (e.g. except Exception, e) W0703
• performance analysis
• Are there any warnings (deprecated things, missing permissions)
• Check if button object functions return dicts/ir.actions
• Presence of i18n/module_name.pot
• OCA is in the Author of Module
• multiple author are in correct csv format in Module
• Module description is as README.rst with OCA template
• pep8-naming

mainly update the doc and have the sample configuration point there.

Current https://github.com/OCA/server-tools shows green for server tests, but you can see in the log a traceback ValueError: External ID not found in the system: base.module_mi

Seen in this log: https://travis-ci.org/dreispt/server-tools/jobs/35732685

There is no reason to have before install unless you are doing sudo apt-get commands

Currently there is a ODOO_REPO variable environment.
But there isn't a MODULE_REPOS environment.
Sometimes is required more than one modules repo.
And sometimes is required more than one main repo too.

• MODULES REPOS

  • Using module repos
    You can Refactory this line https://github.com/OCA/maintainer-quality-tools/blob/master/.travis.yml#L26
    to MODULE_REPOS=https://github.com/OCA/maintainer-quality-tools.git:(other ones)
    And change MQT script to download this repos and copy to addons path of instance.

  • Using modules to install
    With previous change you can set many MODULE_REPOS to test it.
    However, maybe produce a error because you don't need test all modules of a repo.
    Then we will need a new var to set modules to test.

    For example: MODULE_TO_INSTALL=account,(other ones)

  • Using modules app in your repo (recommend)
    A similar previous structure we are using with runbot.
    However, we will need specify all list of modules to test in env.
    We propose add into your repository a module_app. (Not required, only we recommend you)

    For example:
    Basic module to use odoo-mexico:
    Basic module to use odoo-mexico plus all default modules:

    Now, MODULE_TO_INSTALL=oml_all
    Example module to install in runbot

• MAIN REPOS

  • Using main repos

    Currently we are using a unique var for this case: ODOO_REPO
    Sometimes is required use many main repos similar to many modules repo to copy in main path of instance.
    This is usefully when you need set a group of files or folder in main path.
    For example: MAIN_REPO=$ODOO_REPO:other_repo
    If you need just once repo, then set this var as currently you use it.
    Example list of repositories in runbot

We can make this changes in a new PR.
Only I need you feedback to start.

Often times when developing we forget to test as a non-super user.

A common issue from this is that creating the security csv is often forgotten.

The result is that when users test, they have no access and no way of gaining it.

Odoo will raise a warning when this happens, and it can be grepped.

This is what the warning looks like:

2014-08-04 04:12:58,385 28550 WARNING database openerp.modules.loading: The model account.config.settings has no access rules, consider adding one. E.g. access_account_config_settings,access_account_config_settings,model_account_config_settings,,1,1,1,1

False positives (should have failed)

Can be fixed by exiting with error code of executable instead of exiting with 0 if the grep didn't find any errors:

• https://travis-ci.org/OCA/hr/builds/30553566
• https://travis-ci.org/OCA/webkit-tools/jobs/31328318#L1024

See #50

• https://travis-ci.org/mga-team-odoo/jasperserver/jobs/31813285 (#60)

False negatives (should have passed)

• https://travis-ci.org/OCA/purchase-workflow/jobs/31595730

  Probably due to the fact that there were no modules to install

I just found out about #90 : an option to test modules separately. I think it is a very good idea in all cases, and I thank you all for making it work.

I'm only a bit concerned about the naming: unit test is something else, this option is about running all the tests of every module separately.

What about renaming it to something like ISOLATE_MODULES or something about that? Keeping the old option for compatibility.

Sorry for arriving late into the discussion, I completely missed #90.

I have seen issues introduced by PR #70 : see for instance https://travis-ci.org/OCA/sale-workflow/jobs/33466183 : the init.py files in unported are now checked, and they are also checked by the second flake8 run which is supposed to ignore init.py files.

See https://travis-ci.org/OCA/account-financial-tools/jobs/34104449
account_constraints is within the EXCLUDE list but:

2014-09-01 14:11:11,223 23257 INFO openerp_test openerp.modules.module: module account_constraints: creating or updating database tables
2014-09-01 14:11:11,565 23257 INFO openerp_test openerp.modules.loading: module account_constraints: loading view/account_journal.xml

thus it raises

File "/home/travis/build/OCA/account-financial-tools/account_constraints/account_constraints.py", line 90, in _check_invoice_related_move
'invoice directly.\n%s.') % err_msg
except_osv: (u'Error', u'You cannot do this on an entry generated by an invoice. You must change the related invoice directly.\nInvoice name (id): False (3).')

odoo process next parameter
--load-language=

.travis we can set a var env to set this value.

Maybe you want a log of language

@nbessi pointed out a relevant concern:

There is no reason not to have announcement and release cycle on quality tools as it impacts everybody work. And given people some air between releases that may generate rework.

It's true most people won't be following the discussions here, so they'll be unaware of the changes affecting their test builds.
I agree that at least an announcement on recent and shortly upcoming changes should be made.

Thoughts?

Specific pattern to exclude can not be specified in run flake 8.

For example the use of oerpscenario/behave (DSL) to provide test or initialization data will break build.
I'm also thinking of some captured data to make interface mock test that should be ignored by flake8.

Regards

See for example
https://travis-ci.org/OCA/stock-logistics-warehouse/jobs/34939684

I'm preparing a fix

flake8 is checking for a redefinition of a method which is generally fine, but is required by the new odoo API when we want a new and old API compatibility :

Exemple in account: https://github.com/odoo/odoo/blob/8.0/addons/account/account_invoice.py#L1107-L1196

I have an example in a community module too: https://github.com/guewen/bank-statement-reconcile/blob/0b84dda3ff6f58c07511928add4a9af0eba69849/account_invoice_reference/account_invoice.py#L28-L38

flake8 . --config=${FLAKE8_CONFIG_DIR}/travis_run_flake8.cfg

./account_invoice_reference/account_invoice.py:36:5: F811 redefinition of unused '_ref_from_invoice' from line 28

Should we remove this check?