hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Peter Vary <pv...@cloudera.com>
Subject Re: [DISCUSS] Yetus pre-commit tests
Date Mon, 14 Nov 2016 09:20:09 GMT
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 <sseth@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 <mailto:pvary@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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message