Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Classifying junit tests for testing versatility #4896

Closed
matthew-a-dunlap opened this issue Jul 26, 2018 · 23 comments · Fixed by #4989
Closed

Classifying junit tests for testing versatility #4896

matthew-a-dunlap opened this issue Jul 26, 2018 · 23 comments · Fixed by #4989

Comments

@matthew-a-dunlap
Copy link
Contributor

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
Copy link
Contributor

pameyer commented Aug 3, 2018

@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
Copy link
Contributor Author

matthew-a-dunlap commented Aug 3, 2018

@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
Copy link
Contributor

pameyer commented Aug 3, 2018

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

@djbrooke
Copy link
Contributor

djbrooke commented Aug 6, 2018

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

@benjamin-martinez
Copy link
Contributor

@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
Copy link
Contributor

One good category is test that depend on external resources

@benjamin-martinez
Copy link
Contributor

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
Copy link
Contributor Author

matthew-a-dunlap commented Aug 8, 2018

@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
Copy link
Member

pdurbin commented Aug 9, 2018

@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
Copy link
Contributor

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
Copy link
Member

pdurbin commented Aug 21, 2018

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.

@kcondon kcondon self-assigned this Aug 21, 2018
kcondon added a commit that referenced this issue Aug 21, 2018
add Maven profiles: default excludes non-essential unit tests #4896
@kcondon kcondon closed this as completed Aug 21, 2018
@pdurbin
Copy link
Member

pdurbin commented Aug 23, 2018

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:

screen shot 2018-08-23 at 9 54 09 am

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.

@pdurbin pdurbin reopened this Aug 23, 2018
@kcondon
Copy link
Contributor

kcondon commented Aug 23, 2018

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

@kcondon kcondon assigned pdurbin and unassigned kcondon Aug 23, 2018
@pdurbin
Copy link
Member

pdurbin commented Aug 23, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants