Smalltalk › Pharo › Pharo Smalltalk Developers

Release test for unused temps and other

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

7 messages Options

Torsten Bergmann

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.

gcotelli

Re: Release test for unused temps and other

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 <[hidden email]> 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.

CyrilFerlicot

Re: Release test for unused temps and other

In reply to this post by Torsten Bergmann

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

signature.asc (836 bytes) Download Attachment

gcotelli

Re: Release test for unused temps and other

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

alistairgrant

Re: Release test for unused temps and other

In reply to this post by CyrilFerlicot

On Thu, 6 Sep 2018 at 23:06, Cyril Ferlicot D. <[hidden email]> 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

Guillermo Polito

Re: Release test for unused temps and other

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 <[hidden email]> wrote:

On Thu, 6 Sep 2018 at 23:06, Cyril Ferlicot D. <[hidden email]> 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

Web: http://guillep.github.io

Phone: +33 06 52 70 66 13

Torsten Bergmann

Re: Release test for unused temps and other

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