Precommit functionality that loops changed test classes

[Andrew Wang <an...@cloudera.com>]

Hi folks,

I'd like a some new precommit functionality that runs new or changed test
classes repeatedly, and -1's on the first failure. The idea is to prevent
the addition of new flaky tests.

I looked at the existing whitespace.sh and maven.sh for ideas, but I'm a
bit lost. I think the basic idea would be:

* add a new "changed_workers" function to test-patch.sh similar to
modules_workers that finds the changed test files and runs them in a loop.
This should be "-fae" style so we find all flaky test classes.
* add a new "loopchanged" test type to maven.sh, then a postcompile step
that invokes changed_workers

Is this basically right? If someone more familiar knows exactly how to do
this already, I'm also happy to review.

Best,
Andrew

Re: Precommit functionality that loops changed test classes

[Andrew Wang <an...@cloudera.com>]

Thanks for the reply Allen. Would this functionality fit better as a Maven
plugin? Surefire does have a provision for rerunning failed tests that
could possibly be extended for my usecase:

http://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#rerunFailingTestsCount

Yetus would be responsible for passing in the list of changed test classes.
Looking for the default Surefire test name pattern has been reliable for
Hadoop at least, and Surefire filters out abstract classes and classes that
don't have any @Test annotations.

On Mon, Apr 17, 2017 at 8:19 PM, Allen Wittenauer <aw...@effectivemachines.com>
wrote:

>
> > On Apr 17, 2017, at 10:41 AM, Andrew Wang <an...@cloudera.com>
> wrote:
> >
> > Hi folks,
> >
> > I'd like a some new precommit functionality that runs new or changed test
> > classes repeatedly, and -1's on the first failure. The idea is to prevent
> > the addition of new flaky tests.
>
>         It's doable, but it's not a trivial plugin to write.  It's
> effectively a very specialized clone of check_unittests.   It's also
> important to recognize that it can't run forever; it is going to need to
> have an exit criteria of some sort. (run 10x or whatever... it's going to
> be expensive on certain tests.)
>
> >
> > I looked at the existing whitespace.sh and maven.sh for ideas, but I'm a
> > bit lost. I think the basic idea would be:
> >
> > * add a new "changed_workers" function to test-patch.sh similar to
> > modules_workers that finds the changed test files and runs them in a
> loop.
> > This should be "-fae" style so we find all flaky test classes.
> > * add a new "loopchanged" test type to maven.sh, then a postcompile step
> > that invokes changed_workers
> >
> > Is this basically right?
>
>         Well, I'd probably avoid adding more stuff to core test-patch.sh
> unless we have really need to.  A lot of the changes over the past year was
> to pull as much out of it as we could.  That said, I'd probably take this
> approach:
>
>         * create a new test type.  It will either need to be maven
> specific or have hooks to do per-build-tool logic.  I'm assuming the former
> from here on out.
>         * Read the patch file for a src/test add/modify and add the file
> name to an array.
>         * Similar to how check_unittest works:
>         * Loop around modules
>                 * Read all of the files for that module out of the array,
> attempting to figure out the names of the tests that actually changed
> (there's no guarantee that filename = test name, unfortunately)
>                 * Call module_workers. You'll want to stuff the extra CLI
> bits that limit which tests as part of the args.  There's no need to
> duplicate module_workers. It can do exactly what you want to do already.
>                 * record the exit status and move to the next test or
> module
>
>         Logging here is going to be tricky due to the way Junit test logs
> are done.  They do not necessarily have to match the method/class/file
> being used.  On a failure, the logs are going to be tucked away specially
> in the artifacts dir.
>
> > If someone more familiar knows exactly how to do
> > this already, I'm also happy to review.
>
>
>

Re: Precommit functionality that loops changed test classes

[Allen Wittenauer <aw...@effectivemachines.com>]

> On Apr 17, 2017, at 10:41 AM, Andrew Wang <an...@cloudera.com> wrote:
> 
> Hi folks,
> 
> I'd like a some new precommit functionality that runs new or changed test
> classes repeatedly, and -1's on the first failure. The idea is to prevent
> the addition of new flaky tests.

	It's doable, but it's not a trivial plugin to write.  It's effectively a very specialized clone of check_unittests.   It's also important to recognize that it can't run forever; it is going to need to have an exit criteria of some sort. (run 10x or whatever... it's going to be expensive on certain tests.)

> 
> I looked at the existing whitespace.sh and maven.sh for ideas, but I'm a
> bit lost. I think the basic idea would be:
> 
> * add a new "changed_workers" function to test-patch.sh similar to
> modules_workers that finds the changed test files and runs them in a loop.
> This should be "-fae" style so we find all flaky test classes.
> * add a new "loopchanged" test type to maven.sh, then a postcompile step
> that invokes changed_workers
> 
> Is this basically right?

	Well, I'd probably avoid adding more stuff to core test-patch.sh unless we have really need to.  A lot of the changes over the past year was to pull as much out of it as we could.  That said, I'd probably take this approach:

	* create a new test type.  It will either need to be maven specific or have hooks to do per-build-tool logic.  I'm assuming the former from here on out.
	* Read the patch file for a src/test add/modify and add the file name to an array.
	* Similar to how check_unittest works:
	* Loop around modules
		* Read all of the files for that module out of the array, attempting to figure out the names of the tests that actually changed (there's no guarantee that filename = test name, unfortunately) 
		* Call module_workers. You'll want to stuff the extra CLI bits that limit which tests as part of the args.  There's no need to duplicate module_workers. It can do exactly what you want to do already.
		* record the exit status and move to the next test or module

	Logging here is going to be tricky due to the way Junit test logs are done.  They do not necessarily have to match the method/class/file being used.  On a failure, the logs are going to be tucked away specially in the artifacts dir.

> If someone more familiar knows exactly how to do
> this already, I'm also happy to review.