Re: [tryton-dev] Adding flake8 and coverage on tox and CI
On 07 Feb 18:13, Sergi Almacellas Abellana wrote: > El 07/02/15 a les 09:24, Cédric Krier ha escrit: > >On 07 Feb 12:34, Sharoon Thomas wrote: > >>>On 02/06, Sergi Almacellas Abellana wrote: > >Hi, > > > >As a PoC i just added flake8 check to tox [1] on one of my personal > >projects. The output is available on drone [2]. > > > >I tought it my be interesting to have this check also on all tryton > >modules. > >What do you think? . > >>> > >>>We at Openlabs have been using this for our modules since 2 years. > >>>Having a check on Flake8 brings consistency to the codebase and makes it > >>>easier for new contributors to understand the coding standards expected > >>>instead of a human doing it. > >We already have it on codereview so nothing new. > >And such check in after commit is just too late. > > I propose it because i imagine that some point in the future we will have > tests run on codereview so it's easier to have all information in one place. As far as there is not correct coverage support in drone, for me it is useless. https://github.com/drone/drone/issues/440 https://github.com/drone/drone/issues/239 > >Also this will allow to deprecate reviewbot once all codereviews are > >tested > >on drone. > >>>Having flake8 tests on CI is certainly better than having it on the review > >>>tool. > >No, it is too late and false positive will be such a pain. > >More over, our reviewbot is doing a great job by commenting inline > >flake8 issues instead of having a plain report somewhere else. > > But it also reports false positives (unchanged code), so it's the same > problem. When I talk about false positive, I talk about CI failure because of flake8 issue or wrong interpretation or coding exception etc. Of course, it is a little bit annoying to have false positive with reviewbot but I don't think it is possible to improve it because some "non-changed line" comment could be valid comment. I just wanted to say that having it inlineis much better than a plain report somewhere else. -- Cédric Krier - B2CK SPRL Email/Jabber: cedric.kr...@b2ck.com Tel: +32 472 54 46 59 Website: http://www.b2ck.com/
Re: [tryton-dev] Adding flake8 and coverage on tox and CI
On 07 Feb 14:25, Sharoon Thomas wrote: > On 02/07, Cédric Krier wrote: > > On 07 Feb 12:34, Sharoon Thomas wrote: > > > On 02/06, Sergi Almacellas Abellana wrote: > > > > I also managed to run coverage to ensure that module coverage is over a > > > > specific % (75% on my case, but can be customized per module). Patch is > > > > here[3] (and here[4] is run by default). The output is also available on > > > > drone [5]. Do you think this is also interesting to add? > > > > > > Coverage % could be a vanity metric but we have been trying to achieve > > > 100% coverage for critical modules and a significantly high coverage > > > for others. This has certainly improved the quality of the code and if > > > the tests are written well (not just for the sake of coverage) there are > > > hardly any maintenance issues. > > > > Base on what? Where do you find a correlation between coverage and > > quality? How do you achieve "not just for the sake of coverage"? Which > > threshold is it? > > Statement coverage testing is a good measure of what **isn't** being tested > in your code. Knowing what is tested and what is not by a test suite and > seeing if a patch reduces code coverage is important to maintaining the > quality of the codebase. As you said it is a *measure* so it doesn't have its place on CI. > I hope you remember that there were branches of code in the ModelSQL.write > which could never be reached when we tested for coverage. So what? What is your plan to improve that? "Running coverage"? Why don't you run it now? > > What the point of having failure like that: > > > > https://coveralls.io/r/openlabs/nereid-webshop > > This is not a failure. OK so a big red label is not a failure. So this tools is even worst than what I thought. > > https://travis-ci.org/openlabs/nereid-webshop/jobs/49836710 > > Those have to be fixed. Flake8 had an update today. Flake8 gives us a > coding style to follow that most python programmers can follow. It is > not what Cedrik thinks is beautiful or I think is beautiful. This makes flake8 requirement even worst, I don't want to have to change code to please flake8. And adding flake8 to CI will make required to do that ASAP otherwise the CI will be useless by reporting false failing. > > As usual you are just following to mass without thinking exactly what it > > implies and what it brings. > > I'd prefer "Tested code that works" and "Tested code that breaks" to > "Untested code that works". Without coverage you have no way of knowing > which part is tested to begin with. You are welcomed to run coverage in your developement process. I even encourage everyone to do it because it is one of the possible measure that can give you some information about the quality. > If preferring to know which part of > the code is tested is "mass thinking", I guess I am with the mass and > not with you. You don't understand at all my thinking. I never said to not run coverage tools on trytond. I said it is not the job of the CI, it must not fail the CI, it must not prevent to develop. > PS: I don't intend on changing how you think about this. So you admit to not have good argument. > This is a > public mailing list About Tryton developement and nothing else. > and my mail only explains why we use coverage and > flake8 to provide better quality software to our customers. Which is clearly showed on the very first repository I pick from you. > If you think > these tools don't help you in improving software quality, so be it. Never say that, re-read my emails. PS: My name is Cédric. -- Cédric Krier - B2CK SPRL Email/Jabber: cedric.kr...@b2ck.com Tel: +32 472 54 46 59 Website: http://www.b2ck.com/ pgp7GbXmbDmsf.pgp Description: PGP signature
Re: [tryton-dev] Adding flake8 and coverage on tox and CI
On 07 Feb 18:09, Sergi Almacellas Abellana wrote: > For me this will prevent to add a lot of new more code that won't be tested. > And that's my objective. This is acheive by peer reviewing like any other quality control. -- Cédric Krier - B2CK SPRL Email/Jabber: cedric.kr...@b2ck.com Tel: +32 472 54 46 59 Website: http://www.b2ck.com/
Re: [tryton-dev] Adding flake8 and coverage on tox and CI
El 07/02/15 a les 09:24, Cédric Krier ha escrit: On 07 Feb 12:34, Sharoon Thomas wrote: >On 02/06, Sergi Almacellas Abellana wrote: > >Hi, > > > >As a PoC i just added flake8 check to tox [1] on one of my personal > >projects. The output is available on drone [2]. > > > >I tought it my be interesting to have this check also on all tryton modules. > >What do you think? . > >We at Openlabs have been using this for our modules since 2 years. >Having a check on Flake8 brings consistency to the codebase and makes it >easier for new contributors to understand the coding standards expected >instead of a human doing it. We already have it on codereview so nothing new. And such check in after commit is just too late. I propose it because i imagine that some point in the future we will have tests run on codereview so it's easier to have all information in one place. > >Also this will allow to deprecate reviewbot once all codereviews are tested > >on drone. >Having flake8 tests on CI is certainly better than having it on the review tool. No, it is too late and false positive will be such a pain. More over, our reviewbot is doing a great job by commenting inline flake8 issues instead of having a plain report somewhere else. But it also reports false positives (unchanged code), so it's the same problem. Also what the point about having such thing in the code except just making it harder to read: https://bitbucket.org/pokoli/trytond-buti/commits/60d7a13855325f9069d85db15ab7421edb959922#L__init__.pyT5 > >I also managed to run coverage to ensure that module coverage is over a > >specific % (75% on my case, but can be customized per module). Patch is > >here[3] (and here[4] is run by default). The output is also available on > >drone [5]. Do you think this is also interesting to add? > >Coverage % could be a vanity metric but we have been trying to achieve >100% coverage for critical modules and a significantly high coverage >for others. This has certainly improved the quality of the code and if >the tests are written well (not just for the sake of coverage) there are >hardly any maintenance issues. Base on what? Where do you find a correlation between coverage and quality? How do you achieve "not just for the sake of coverage"? Which threshold is it? What the point of having failure like that: https://coveralls.io/r/openlabs/nereid-webshop https://travis-ci.org/openlabs/nereid-webshop/jobs/49836710 As usual you are just following to mass without thinking exactly what it implies and what it brings. -- Sergi Almacellas Abellana www.koolpi.com Twitter: @pokoli_srk
Re: [tryton-dev] Adding flake8 and coverage on tox and CI
El 06/02/15 a les 17:29, Cédric Krier ha escrit: On 06 Feb 16:16, Sergi Almacellas Abellana wrote: Hi, As a PoC i just added flake8 check to tox [1] on one of my personal projects. The output is available on drone [2]. I tought it my be interesting to have this check also on all tryton modules. What do you think? . Also this will allow to deprecate reviewbot once all codereviews are tested on drone. Note: Adding flake8 to tox, means fixing all errors (otherwise build will fail on drone) which means: adding # noqa to all __init__.py lines that import * adding # noqa to all tests/__init__.py Fix all possible errors. I'm strongly against that. flake8 is not the holly grail. Also I'm against having build failure because flake8 thinks you did not correctly indent something or you should write differently something. Please re-read PEP-8, it is just a suggestion. I don't like to add the coments also, so maybe we need to be more flexible on this files. Maybe ignoring __init__.py files and the identation error which is not well detected by pyflakes will do the rick. I also managed to run coverage to ensure that module coverage is over a specific % (75% on my case, but can be customized per module). Patch is here[3] (and here[4] is run by default). The output is also available on drone [5]. Do you think this is also interesting to add? Only if it is output and the build will not fail because coverage is lower. It can be done without a problem, but i imagine this will be only noise. But I also fear that it will just be noise and resource consuming. Also such constraint will lead to adding useless tests to increase the coverage and thus making maintenance on tests more expensive. For me this will prevent to add a lot of new more code that won't be tested. And that's my objective. For me, quality is not achieve by tools but by the process/workflow used (like codereview, bugtracking, non-regression test, documenation, comments etc.). The tools like flake8 or coverage are only tools to help to follow the process but they must not become the target otherwise we miss the real point which is quality. -- Sergi Almacellas Abellana www.koolpi.com Twitter: @pokoli_srk
Re: [tryton-dev] Adding flake8 and coverage on tox and CI
On 02/07, Cédric Krier wrote: > On 07 Feb 12:34, Sharoon Thomas wrote: > > On 02/06, Sergi Almacellas Abellana wrote: > > > I also managed to run coverage to ensure that module coverage is over a > > > specific % (75% on my case, but can be customized per module). Patch is > > > here[3] (and here[4] is run by default). The output is also available on > > > drone [5]. Do you think this is also interesting to add? > > > > Coverage % could be a vanity metric but we have been trying to achieve > > 100% coverage for critical modules and a significantly high coverage > > for others. This has certainly improved the quality of the code and if > > the tests are written well (not just for the sake of coverage) there are > > hardly any maintenance issues. > > Base on what? Where do you find a correlation between coverage and > quality? How do you achieve "not just for the sake of coverage"? Which > threshold is it? Statement coverage testing is a good measure of what **isn't** being tested in your code. Knowing what is tested and what is not by a test suite and seeing if a patch reduces code coverage is important to maintaining the quality of the codebase. I hope you remember that there were branches of code in the ModelSQL.write which could never be reached when we tested for coverage. > > What the point of having failure like that: > > https://coveralls.io/r/openlabs/nereid-webshop This is not a failure. Shows us in a pull-request if the patch reduces code coverage significantly. As your link shows minor fluctuations are fine. We aren't relegious about 100% coverage. > https://travis-ci.org/openlabs/nereid-webshop/jobs/49836710 Those have to be fixed. Flake8 had an update today. Flake8 gives us a coding style to follow that most python programmers can follow. It is not what Cedrik thinks is beautiful or I think is beautiful. > As usual you are just following to mass without thinking exactly what it > implies and what it brings. I'd prefer "Tested code that works" and "Tested code that breaks" to "Untested code that works". Without coverage you have no way of knowing which part is tested to begin with. If preferring to know which part of the code is tested is "mass thinking", I guess I am with the mass and not with you. PS: I don't intend on changing how you think about this. This is a public mailing list and my mail only explains why we use coverage and flake8 to provide better quality software to our customers. If you think these tools don't help you in improving software quality, so be it. Thanks & Regards Sharoon Thomas pgpUhAOEHpoeT.pgp Description: PGP signature
Re: [tryton-dev] Adding flake8 and coverage on tox and CI
On 07 Feb 12:34, Sharoon Thomas wrote: > On 02/06, Sergi Almacellas Abellana wrote: > > Hi, > > > > As a PoC i just added flake8 check to tox [1] on one of my personal > > projects. The output is available on drone [2]. > > > > I tought it my be interesting to have this check also on all tryton modules. > > What do you think? . > > We at Openlabs have been using this for our modules since 2 years. > Having a check on Flake8 brings consistency to the codebase and makes it > easier for new contributors to understand the coding standards expected > instead of a human doing it. We already have it on codereview so nothing new. And such check in after commit is just too late. > > Also this will allow to deprecate reviewbot once all codereviews are tested > > on drone. > Having flake8 tests on CI is certainly better than having it on the review > tool. No, it is too late and false positive will be such a pain. More over, our reviewbot is doing a great job by commenting inline flake8 issues instead of having a plain report somewhere else. Also what the point about having such thing in the code except just making it harder to read: https://bitbucket.org/pokoli/trytond-buti/commits/60d7a13855325f9069d85db15ab7421edb959922#L__init__.pyT5 > > I also managed to run coverage to ensure that module coverage is over a > > specific % (75% on my case, but can be customized per module). Patch is > > here[3] (and here[4] is run by default). The output is also available on > > drone [5]. Do you think this is also interesting to add? > > Coverage % could be a vanity metric but we have been trying to achieve > 100% coverage for critical modules and a significantly high coverage > for others. This has certainly improved the quality of the code and if > the tests are written well (not just for the sake of coverage) there are > hardly any maintenance issues. Base on what? Where do you find a correlation between coverage and quality? How do you achieve "not just for the sake of coverage"? Which threshold is it? What the point of having failure like that: https://coveralls.io/r/openlabs/nereid-webshop https://travis-ci.org/openlabs/nereid-webshop/jobs/49836710 As usual you are just following to mass without thinking exactly what it implies and what it brings. -- Cédric Krier - B2CK SPRL Email/Jabber: cedric.kr...@b2ck.com Tel: +32 472 54 46 59 Website: http://www.b2ck.com/ pgp8KuiuVAl2T.pgp Description: PGP signature
Re: [tryton-dev] Adding flake8 and coverage on tox and CI
On 02/06, Sergi Almacellas Abellana wrote: > Hi, > > As a PoC i just added flake8 check to tox [1] on one of my personal > projects. The output is available on drone [2]. > > I tought it my be interesting to have this check also on all tryton modules. > What do you think? . We at Openlabs have been using this for our modules since 2 years. Having a check on Flake8 brings consistency to the codebase and makes it easier for new contributors to understand the coding standards expected instead of a human doing it. > > Also this will allow to deprecate reviewbot once all codereviews are tested > on drone. Having flake8 tests on CI is certainly better than having it on the review tool. > I also managed to run coverage to ensure that module coverage is over a > specific % (75% on my case, but can be customized per module). Patch is > here[3] (and here[4] is run by default). The output is also available on > drone [5]. Do you think this is also interesting to add? Coverage % could be a vanity metric but we have been trying to achieve 100% coverage for critical modules and a significantly high coverage for others. This has certainly improved the quality of the code and if the tests are written well (not just for the sake of coverage) there are hardly any maintenance issues. Thanks & Regards Sharoon Thomas pgpTFMxgoQR0B.pgp Description: PGP signature
Re: [tryton-dev] Adding flake8 and coverage on tox and CI
On 06 Feb 16:16, Sergi Almacellas Abellana wrote: > Hi, > > As a PoC i just added flake8 check to tox [1] on one of my personal > projects. The output is available on drone [2]. > > I tought it my be interesting to have this check also on all tryton modules. > What do you think? . > > Also this will allow to deprecate reviewbot once all codereviews are tested > on drone. > > Note: Adding flake8 to tox, means fixing all errors (otherwise build will > fail on drone) which means: > > adding # noqa to all __init__.py lines that import * > adding # noqa to all tests/__init__.py > Fix all possible errors. I'm strongly against that. flake8 is not the holly grail. Also I'm against having build failure because flake8 thinks you did not correctly indent something or you should write differently something. Please re-read PEP-8, it is just a suggestion. > I also managed to run coverage to ensure that module coverage is over a > specific % (75% on my case, but can be customized per module). Patch is > here[3] (and here[4] is run by default). The output is also available on > drone [5]. Do you think this is also interesting to add? Only if it is output and the build will not fail because coverage is lower. But I also fear that it will just be noise and resource consuming. Also such constraint will lead to adding useless tests to increase the coverage and thus making maintenance on tests more expensive. For me, quality is not achieve by tools but by the process/workflow used (like codereview, bugtracking, non-regression test, documenation, comments etc.). The tools like flake8 or coverage are only tools to help to follow the process but they must not become the target otherwise we miss the real point which is quality. -- Cédric Krier - B2CK SPRL Email/Jabber: cedric.kr...@b2ck.com Tel: +32 472 54 46 59 Website: http://www.b2ck.com/
