You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Peter Vary <pv...@cloudera.com> on 2016/11/10 15:57:40 UTC
[DISCUSS] Yetus pre-commit tests
Hi there,
Previously we discussed that it would be good to integrate some automated checks to the pre-commit flow.
Alan Gates suggested Apache Yetus and I checked what it can do for us (Yetus 0.3.0).
The good things that I have found:
Several existing tests (asflicense, author, checkstyle, findbugs, javac, javadoc, test4tests, unitveto, whitespace, xml, junit)
It shows changes in errors/failures so we do not have to clean up the original code, but new code will be checked.
Used by multiple ASF projects already - so we will be Apache conform using it.
Extensible, so if we decide to add the ptest framework to these test this could be done
It is possible to run the test only on the modules which contain changed files
The bad thing is it could take long time to run the tests even with patches touching a single module.
I think we should decide on which test to include into our pre-commit flow based on our needs and the resource requirements. For reference I have run the test for a fairly small patch on my macbook pro 2 times:
Adding 3 new files to the beeline module (1 java, 1 xml, 1 q.out) - took ~4 mins - see the result in the attached beeline.out file
Adding 3 new files (same as before) to the ql module (1 java, 1 xml, 1 q.out) - took ~12 mins - see the result in the attached ql.out file
In nutshell, the out of the box tests which are available in Yetus are (the numbers are the time in seconds required to run the test in beeline/ql plugin):
asflicense (24/23) - apache-rat:check - currently this runs for the full path
author (0/0) - Checks for @author tags
checkstyle (31/66) - checkstyle:checksyle
findbugs (73/353) - findbugs:findbugs
javac (53/147) - install compilation warnings (the runtime presented in the tables are not valid)
javadoc (34/92) - javadoc warnings
test4tests (0/0) - checks if there is any test changed
unitveto (0/0) - checks for files in patch
whitespace (1/2) - tabs, whitespaces at the end of the line
xml (1/1) - xml basic validation
junit - running junit tests - we will use ptest anyway, so not played with this
I would like to know your opinion on which test should we enable, and which test should we leave out in our pre-commit workflow.
Thanks,
Peter
Re: [DISCUSS] Yetus pre-commit tests
Posted by Peter Vary <pv...@cloudera.com>.
Hi Sid,
Good to know, that we have idle resources to run the checks. If it is done in parallel then I think it should be possible to run all relevant tests you mentioned. It will take some time to make it work, since I will try to contact the Yetus project to check if it is possible to incorporate the changes we need, so we do not have to have our “own” Yetus.
As for the checkstyle test, it will help to check the Java code that adheres to a coding standard. Our one is defined here: https://cwiki.apache.org/confluence/display/Hive/HowToContribute#HowToContribute-CodingConventions
"Code should be formatted according to Sun's conventions <http://web.archive.org/web/20140228225807/http://www.oracle.com/technetwork/java/codeconventions-150003.pdf>, with two exceptions:
Indent two (2) spaces per level, not four (4).
Line length limit is 100 chars, instead of 80 chars."
There is an existing checkstyle configuration which I will compare with the standard sun checkstyle configuration to see the differences.
My plan for the rollout is the following:
Create a working version which could be run from command line
Review and commit it - we do not automate it this time. I would like to make it available for others to test too.
Open an umbrella jira to collect the problems. Others can add problems here, and I will run the tests for multiple patches.
We can discuss the intended changes, and make them
After we are satisfied with the check we should enable in on jenkins
I would like to create tests which we are able to keep all green, so if there is a -1 in any one aspect then it should clearly indicate that there is a valid problem. I am ready to define a little more lax rules for it if needed. If we can do this with the pre-commit tests, and with the ptests as well, then I think we made a big step forward to an even better quality project.
Do we agree on this rollout plan, do anyone have any suggestions or ideas to consider?
Thanks,
Peter
> On Nov 10, 2016, at 10:29 PM, Siddharth Seth <ss...@apache.org> wrote:
>
> Peter
> In terms of the modules - ignoring the time taken - I would vote for
> asflicense, author, findbugs, javac, maybe javadoc, wrhitespace. Not sure
> what checkstyle does, and some form of test4tests is already covered in
> ptest. This will at least help preventing new issues. Fixing the existing
> set would be quite an exercise.
>
> The numbers that you have posted - I think they are on your local system?
> I'd expect these to be higher on the build machines. Not too keen on having
> the runtime go up by 10+ minutes though. Would this run before ptest is
> actually started? Is it possible to start this from within ptest as a
> parallel phase? The ptest server doesn't do much while tests are running.
> Running the regular ptest flow, and this set of checks could be
> parallelized there.
>
> Thank you for taking this up.
>
> Sid
>
> On Thu, Nov 10, 2016 at 7:57 AM, Peter Vary <pvary@cloudera.com <ma...@cloudera.com>> wrote:
>
>> Hi there,
>>
>> Previously we discussed that it would be good to integrate some automated
>> checks to the pre-commit flow.
>> Alan Gates suggested Apache Yetus and I checked what it can do for us
>> (Yetus 0.3.0).
>>
>> The good things that I have found:
>>
>> - Several existing tests (asflicense, author, checkstyle, findbugs,
>> javac, javadoc, test4tests, unitveto, whitespace, xml, junit)
>> - It shows changes in errors/failures so we do not have to clean up
>> the original code, but new code will be checked.
>> - Used by multiple ASF projects already - so we will be Apache conform
>> using it.
>> - Extensible, so if we decide to add the ptest framework to these test
>> this could be done
>> - It is possible to run the test only on the modules which contain
>> changed files
>>
>> The bad thing is it could take long time to run the tests even with
>> patches touching a single module.
>>
>> I think we should decide on which test to include into our pre-commit flow
>> based on our needs and the resource requirements. For reference I have run
>> the test for a fairly small patch on my macbook pro 2 times:
>>
>> 1. Adding 3 new files to the beeline module (1 java, 1 xml, 1 q.out) -
>> took ~4 mins - see the result in the attached beeline.out file
>> 2. Adding 3 new files (same as before) to the ql module (1 java, 1
>> xml, 1 q.out) - took ~12 mins - see the result in the attached ql.out file
>>
>> In nutshell, the out of the box tests which are available in Yetus are
>> (the numbers are the time in seconds required to run the test in beeline/ql
>> plugin):
>>
>> - asflicense (24/23) - apache-rat:check - currently this runs for the
>> full path
>> - author (0/0) - Checks for @author tags
>> - checkstyle (31/66) - checkstyle:checksyle
>> - findbugs (73/353) - findbugs:findbugs
>> - javac (53/147) - install compilation warnings (the runtime presented
>> in the tables are not valid)
>> - javadoc (34/92) - javadoc warnings
>> - test4tests (0/0) - checks if there is any test changed
>> - unitveto (0/0) - checks for files in patch
>> - whitespace (1/2) - tabs, whitespaces at the end of the line
>> - xml (1/1) - xml basic validation
>> - junit - running junit tests - we will use ptest anyway, so not
>> played with this
>>
>>
>> I would like to know your opinion on which test should we enable, and
>> which test should we leave out in our pre-commit workflow.
>>
>> Thanks,
>> Peter
Re: [DISCUSS] Yetus pre-commit tests
Posted by Siddharth Seth <ss...@apache.org>.
Peter
In terms of the modules - ignoring the time taken - I would vote for
asflicense, author, findbugs, javac, maybe javadoc, wrhitespace. Not sure
what checkstyle does, and some form of test4tests is already covered in
ptest. This will at least help preventing new issues. Fixing the existing
set would be quite an exercise.
The numbers that you have posted - I think they are on your local system?
I'd expect these to be higher on the build machines. Not too keen on having
the runtime go up by 10+ minutes though. Would this run before ptest is
actually started? Is it possible to start this from within ptest as a
parallel phase? The ptest server doesn't do much while tests are running.
Running the regular ptest flow, and this set of checks could be
parallelized there.
Thank you for taking this up.
Sid
On Thu, Nov 10, 2016 at 7:57 AM, Peter Vary <pv...@cloudera.com> wrote:
> Hi there,
>
> Previously we discussed that it would be good to integrate some automated
> checks to the pre-commit flow.
> Alan Gates suggested Apache Yetus and I checked what it can do for us
> (Yetus 0.3.0).
>
> The good things that I have found:
>
> - Several existing tests (asflicense, author, checkstyle, findbugs,
> javac, javadoc, test4tests, unitveto, whitespace, xml, junit)
> - It shows changes in errors/failures so we do not have to clean up
> the original code, but new code will be checked.
> - Used by multiple ASF projects already - so we will be Apache conform
> using it.
> - Extensible, so if we decide to add the ptest framework to these test
> this could be done
> - It is possible to run the test only on the modules which contain
> changed files
>
> The bad thing is it could take long time to run the tests even with
> patches touching a single module.
>
> I think we should decide on which test to include into our pre-commit flow
> based on our needs and the resource requirements. For reference I have run
> the test for a fairly small patch on my macbook pro 2 times:
>
> 1. Adding 3 new files to the beeline module (1 java, 1 xml, 1 q.out) -
> took ~4 mins - see the result in the attached beeline.out file
> 2. Adding 3 new files (same as before) to the ql module (1 java, 1
> xml, 1 q.out) - took ~12 mins - see the result in the attached ql.out file
>
> In nutshell, the out of the box tests which are available in Yetus are
> (the numbers are the time in seconds required to run the test in beeline/ql
> plugin):
>
> - asflicense (24/23) - apache-rat:check - currently this runs for the
> full path
> - author (0/0) - Checks for @author tags
> - checkstyle (31/66) - checkstyle:checksyle
> - findbugs (73/353) - findbugs:findbugs
> - javac (53/147) - install compilation warnings (the runtime presented
> in the tables are not valid)
> - javadoc (34/92) - javadoc warnings
> - test4tests (0/0) - checks if there is any test changed
> - unitveto (0/0) - checks for files in patch
> - whitespace (1/2) - tabs, whitespaces at the end of the line
> - xml (1/1) - xml basic validation
> - junit - running junit tests - we will use ptest anyway, so not
> played with this
>
>
> I would like to know your opinion on which test should we enable, and
> which test should we leave out in our pre-commit workflow.
>
> Thanks,
> Peter
>
>
>
>