Release test for unused temps and other
Hi, the very nice contribution on GIFReadWriter/animated images (see https://pharo.manuscript.com/f/cases/22137) unfortunately introduced some methods that had no sender, were uncategorized and had unused temporary variables, etc. Something that is meanwhile shown by our tools and easy to fix. I guess it was some overseen leftover. Unfortunately this also slipped through the integration process as it was merged without an approval from a community reviewer. I dont want to lament too much on this specifc case (especially as I now fixed it in https://pharo.fogbugz.com/f/cases/22426). But after I cleaned the P7 image this spring to not have unused temporaries anymore I was suprised to find such glitches again now. So to me it shows that we need to write more tests to detect violations and keep our achieved quality standards up and also should not solely rely on the review process. Otherwise we need to invest our time in cleaning up our own stuff afterwards again and again. To be specific one can evaluate: CompiledMethod allInstances select: [:m | m ast temporaries anySatisfy: [:x | x binding isUnused ]]. to find unused temps - which would be easy to package up into one of the release test to ensure no unused temp vars are left (the tearDown would require an "ASTCache reset"). The problem is that this simple test would take around 20-30 seconds depending on machine and I do not know if it would be a good idea/good solution to integrate. Especially I'm not sure if it is worth the issue or will kill the CI again and again. What do you think about it? Comments appreciated? Thanks T.
Hi Torsten, We experienced some similar problems with our builds at Mercap, because all the tests involving code analysis are orders of magnitude slower than the unit tests. What we do is: - When the programmer closes a unit of work (in that case it will be equivalent to create a PR) we run the tests involving this kind of code analysis only over the changed code - In the global build this tests are run in parallel (this wouldn't solve the pressure over the infrastructure, but improves the troughput of the whole build) I think this kind of tests totally worth the effort. Gabriel On Thu, Sep 6, 2018 at 12:51 PM Torsten Bergmann <astares@gmx.de> wrote:
Hi,
the very nice contribution on GIFReadWriter/animated images (see https://pharo.manuscript.com/f/cases/22137) unfortunately introduced some methods that had no sender, were uncategorized and had unused temporary variables, etc.
Something that is meanwhile shown by our tools and easy to fix. I guess it was some overseen leftover. Unfortunately this also slipped through the integration process as it was merged without an approval from a community reviewer.
I dont want to lament too much on this specifc case (especially as I now fixed it in https://pharo.fogbugz.com/f/cases/22426).
But after I cleaned the P7 image this spring to not have unused temporaries anymore I was suprised to find such glitches again now. So to me it shows that we need to write more tests to detect violations and keep our achieved quality standards up and also should not solely rely on the review process.
Otherwise we need to invest our time in cleaning up our own stuff afterwards again and again.
To be specific one can evaluate:
CompiledMethod allInstances select: [:m | m ast temporaries anySatisfy: [:x | x binding isUnused ]].
to find unused temps - which would be easy to package up into one of the release test to ensure no unused temp vars are left (the tearDown would require an "ASTCache reset").
The problem is that this simple test would take around 20-30 seconds depending on machine and I do not know if it would be a good idea/good solution to integrate. Especially I'm not sure if it is worth the issue or will kill the CI again and again.
What do you think about it? Comments appreciated?
Thanks T.
Le 06/09/2018 à 17:50, Torsten Bergmann a écrit :
Hi,
the very nice contribution on GIFReadWriter/animated images (see https://pharo.manuscript.com/f/cases/22137) unfortunately introduced some methods that had no sender, were uncategorized and had unused temporary variables, etc.
Something that is meanwhile shown by our tools and easy to fix. I guess it was some overseen leftover. Unfortunately this also slipped through the integration process as it was merged without an approval from a community reviewer.
I dont want to lament too much on this specifc case (especially as I now fixed it in https://pharo.fogbugz.com/f/cases/22426).
But after I cleaned the P7 image this spring to not have unused temporaries anymore I was suprised to find such glitches again now. So to me it shows that we need to write more tests to detect violations and keep our achieved quality standards up and also should not solely rely on the review process.
Otherwise we need to invest our time in cleaning up our own stuff afterwards again and again.
To be specific one can evaluate:
CompiledMethod allInstances select: [:m | m ast temporaries anySatisfy: [:x | x binding isUnused ]].
to find unused temps - which would be easy to package up into one of the release test to ensure no unused temp vars are left (the tearDown would require an "ASTCache reset").
The problem is that this simple test would take around 20-30 seconds depending on machine and I do not know if it would be a good idea/good solution to integrate. Especially I'm not sure if it is worth the issue or will kill the CI again and again.
What do you think about it? Comments appreciated?
Hi, Maybe we could have a package with release tests that are not executed by the CI but that are executed at the moment of the feature freeze and some time before the Pharo release to ensure the quality of the code base for tests that are too long?
Thanks T.
-- Cyril Ferlicot https://ferlicot.fr
In my experience this is not a good idea. When you're in the pressure of the release the last thing you want it's to fix things that shouldn't be merged in that form. It sends the wrong message to the people contributing, If you have this fail in your own PR you can easily fix it and learn for it for the next time. But at the time Pharo enter feature freeze probably is someone else who has to fix it. Maintaining code quality is way easier if you keep it in good shape everyday.
Hi,
Maybe we could have a package with release tests that are not executed by the CI but that are executed at the moment of the feature freeze and some time before the Pharo release to ensure the quality of the code base for tests that are too long?
Thanks T.
-- Cyril Ferlicot https://ferlicot.fr
On Thu, 6 Sep 2018 at 23:06, Cyril Ferlicot D. <cyril.ferlicot@gmail.com> wrote:
Maybe we could have a package with release tests that are not executed by the CI but that are executed at the moment of the feature freeze and some time before the Pharo release to ensure the quality of the code base for tests that are too long?
The drawback of this approach is that it relies on the original author going back and submitting a fix in a timely manner (quite possibly after they've moved on to other things). If the tests could be run on a specified set of classes / methods, perhaps we could run them as part of Iceberg, i.e. when going to commit code, these tests are run on the classes in the commit and a warning opened prompting the user to fix them before actually committing. Cheers, Alistair
Hi, I don't think that 20 seconds will affect the overall process :). It will only become a problem if we increment the number of these kind of tests by *a lot*... Another solution is to make some tests run in parallel. But for this to run properly we have to increase the number of slaves in Jenkins, and the bad news is that we reached the current maximum limit (of 5 slaves). We have to talk to the inria people to see what solution they could provide to us for this, because otherwise the infrastructure does not scale for lots of PRs with big jobs. Guille On Fri, Sep 7, 2018 at 8:23 AM Alistair Grant <akgrant0710@gmail.com> wrote:
On Thu, 6 Sep 2018 at 23:06, Cyril Ferlicot D. <cyril.ferlicot@gmail.com> wrote:
Maybe we could have a package with release tests that are not executed by the CI but that are executed at the moment of the feature freeze and some time before the Pharo release to ensure the quality of the code base for tests that are too long?
The drawback of this approach is that it relies on the original author going back and submitting a fix in a timely manner (quite possibly after they've moved on to other things).
If the tests could be run on a specified set of classes / methods, perhaps we could run them as part of Iceberg, i.e. when going to commit code, these tests are run on the classes in the commit and a warning opened prompting the user to fix them before actually committing.
Cheers, Alistair
-- Guille Polito Research Engineer Centre de Recherche en Informatique, Signal et Automatique de Lille CRIStAL - UMR 9189 French National Center for Scientific Research - *http://www.cnrs.fr <http://www.cnrs.fr>* *Web:* *http://guillep.github.io* <http://guillep.github.io> *Phone: *+33 06 52 70 66 13
Thank you all for your feedback - so let's do it with a TestCase added into the image. PR is available, according CI is green and ready for review/integration: https://github.com/pharo-project/pharo/pull/1782 Â
participants (5)
-
Alistair Grant -
Cyril Ferlicot D. -
Gabriel Cotelli -
Guillermo Polito -
Torsten Bergmann