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

Comments

Projects
None yet
7 participants
@matthew-a-dunlap
Copy link
Contributor

matthew-a-dunlap commented Jul 26, 2018

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

pameyer commented Aug 3, 2018

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

@djbrooke

This comment has been minimized.

Copy link
Contributor

djbrooke commented Aug 6, 2018

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

@benjamin-martinez

This comment has been minimized.

Copy link
Contributor

benjamin-martinez commented Aug 7, 2018

@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

This comment has been minimized.

Copy link
Contributor

oscardssmith commented Aug 8, 2018

One good category is test that depend on external resources

@benjamin-martinez

This comment has been minimized.

Copy link
Contributor

benjamin-martinez commented Aug 8, 2018

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

This comment has been minimized.

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

This comment has been minimized.

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 added a commit that referenced this issue Aug 9, 2018

@benjamin-martinez

This comment has been minimized.

Copy link
Contributor

benjamin-martinez commented Aug 16, 2018

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.

@benjamin-martinez benjamin-martinez removed their assignment Aug 16, 2018

benjamin-martinez added a commit that referenced this issue Aug 16, 2018

@pdurbin

This comment has been minimized.

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

Merge pull request #4989 from IQSS/4896-junit-tests2
add Maven profiles: default excludes non-essential unit tests #4896

@kcondon kcondon closed this Aug 21, 2018

@kcondon kcondon removed the Status: QA label Aug 21, 2018

@pdurbin

This comment has been minimized.

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

@pdurbin pdurbin added the Status: QA label Aug 23, 2018

@kcondon

This comment has been minimized.

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

This comment has been minimized.

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
You can’t perform that action at this time.