Classifying junit tests for testing versatility

[matthew-a-dunlap]

There is a need to organize our tests such that only some are run during development while others are run by Travis. Some tests can be pretty slow and running the full suite every time can slow down dev work.

Looking into this a bit, there are at least two JUnit options to choose from: @suite and @category annotations . It may be best to choose the option that does not require annotating every test class, but maybe only annotating the slow ones?

More investigation is needed but either way it shouldn't be more work than adding some annotations and doing some minor edits to the build process and travis process

[pameyer]

@matthew-a-dunlap From a very quick look ~ a year ago, completely disabling running unit tests when building (with maven) didn't result in a significant enough speed-up for me personally to bother building that way. Assuming I'm remembering correctly (which is always in question), ~5-10ss of ~1-2m build.

More efficiency is always good - just advanced warning that there's a chance this may not speed things up all that much (hopefully I'm wrong, or things have changed, though).

[matthew-a-dunlap]

@pameyer if anything I know the provenace parser/validator test were slowing down my builds by ~12 seconds, but they may be the only ones with a real impact

[pameyer]

Good point @matthew-a-dunlap - this pre-dated the prov work.

[djbrooke]

assigning to @matthew-a-dunlap to talk about this at next estimation session

[benjamin-martinez]

@matthew-a-dunlap @pameyer Aside from the prov tests, what other tests do you think would be included in this "Slow Test" or "Non-Essential Test" category?

[oscardssmith]

One good category is test that depend on external resources

[benjamin-martinez]

Okay, so what I am thinking is that we should have a test suite called "NonEssential" and within this suite we'll have two+ specific categories. As of now the two categories will be "Slow" and "External Dependency". The name "Slow" is probably just a place holder for now until I can come up with something more specific.

If we want to go even further with this, we could create several test suites that correlate to the specific part of the code base that your working in so only things that you can affect will be tested.

[matthew-a-dunlap]

@benjamin-martinez I think the best approach is to keep it pretty simple for now with room to add more groupings of tests later. Your approach in the first paragraph seems on that path. Can we also create a suite called "Essential" and have it be everything that isn't "Slow" or "External Dependency"? We'll need that to be what is run by the developer builds every time.

I don't have a good understanding of how suite / category are used together. I'll be in shortly and maybe we can discuss more then.

[pdurbin]

@benjamin-martinez please see ec8aa56 for an example of when @Ignore was added because of a dependency on an external resource.

As I said in sprint planning yesterday, I don't think any of us should obsess over classifying existing tests. We are simply trying to add another tool to our toolbox: the ability to mark a test a slow or dependent on an external resource. The benefit is that developers spend less time on slow tests or tests that require reaching out across the network. There seemed to be agreement yesterday that we want Travis to continue running all tests. So we'll need to modify the script line in our .travis.yml from...

script: mvn -DcompilerArgument=-Xlint:unchecked test

... to something that includes an additional mvn argument (probably starting with -D) to indicate that all tests should be run. We should also document this in the "Testing" page in the dev guide so developers know how to run all tests if they want to.

[benjamin-martinez]

I'm pushing what I have done so far, but I'm not entirely happy with my solution. This may end up costing developers more time time than they're saving in the long run. To declare a test as essential or non-essential, one would have to add a tag (@category(NonEssentialTests.class) or @category(EssentialTests.class)) to the start of their JUNIT test above the @test tag. This way, when the project is cleaned and built, only the tests marked (@category(EssentialTests.class) will run.

In order to run all tests, however, developers will have to comment out the maven-surefire-plugin I used in the pom.xml file. I haven't been able to figure out another way to get the Non-essential tests to run in a non-hack way.

There is also the maven command: mvn install -DrunSuite=**/NonEssentialTestSuite.class -DfailIfNoTests=false that should work, unless I've messed up the syntax.

To whoever code reviews this, the only really important files to look at are NonEssentialTests, NonEssentialTestSuite, EssentialTests, EssentialTestSuite, and the pom.xml file.

[pdurbin]

I couldn't get pull request #4968 working so I just created pull request #4989 which I believe meets the definition of done for this issue. The newer pull request is based on the work done in the older one so thank you for showing the way, @benjamin-martinez ! Moving to code review.

[pdurbin]

I just wanted to mention that I just updated the Jenkins job that build the war file for the phoenix server to run all tests like this:

That is to say the following setting:

• Root POM: pom.xml
• Goals and options: clean -DcompilerArgument=-Xlint:unchecked -P all-unit-tests package

I'm going to drag this back to QA to remind me to mention at standup that I think we should do this whenever we build the war file from Jenkins.

[kcondon]

@pdurbin Is this related to the actual classifying of the tests or is this a new suggestion for additional checks via Jenkins jobs?

[pdurbin]

@kcondon I'll explain at standup but the way I think of it is this:

• Developers and Jenkins used to run all unit tests.
• Now developers run slightly fewer unit tests (new default "dev" Maven profile).
• Jenkins should be reconfigured to use the non-default "all-unit-tests" Maven profile so that it continues to execute the complete unit test suite that it was running before the pull request for this issue was merged.