Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upClassifying junit tests for testing versatility #4896
Comments
djbrooke
added
the
Status: Ready
label
Jul 26, 2018
matthew-a-dunlap
referenced this issue
Jul 26, 2018
Closed
As a researcher, I want my uploaded provenance file to be validated so that I know it's in an expected format #4378
djbrooke
added
the
ready for estimation
label
Aug 1, 2018
This comment has been minimized.
This comment has been minimized.
@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). |
This comment has been minimized.
This comment has been minimized.
@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 |
This comment has been minimized.
This comment has been minimized.
Good point @matthew-a-dunlap - this pre-dated the prov work. |
This comment has been minimized.
This comment has been minimized.
assigning to @matthew-a-dunlap to talk about this at next estimation session |
djbrooke
assigned
matthew-a-dunlap
Aug 6, 2018
benjamin-martinez
self-assigned this
Aug 6, 2018
This comment has been minimized.
This comment has been minimized.
@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? |
This comment has been minimized.
This comment has been minimized.
One good category is test that depend on external resources |
This comment has been minimized.
This comment has been minimized.
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. |
This comment has been minimized.
This comment has been minimized.
@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. |
djbrooke
removed
the
ready for estimation
label
Aug 8, 2018
djbrooke
unassigned
matthew-a-dunlap and
benjamin-martinez
Aug 8, 2018
djbrooke
added
Status: This/Next Sprint
and removed
Status: Ready
labels
Aug 8, 2018
benjamin-martinez
self-assigned this
Aug 8, 2018
benjamin-martinez
added
Status: Development
and removed
Status: This/Next Sprint
labels
Aug 8, 2018
This comment has been minimized.
This comment has been minimized.
@benjamin-martinez please see ec8aa56 for an example of when 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
... to something that includes an additional |
added a commit
that referenced
this issue
Aug 9, 2018
added a commit
that referenced
this issue
Aug 10, 2018
This comment has been minimized.
This comment has been minimized.
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: 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
added
Status: Code Review
and removed
Status: Development
labels
Aug 16, 2018
benjamin-martinez
removed their assignment
Aug 16, 2018
djbrooke
assigned
matthew-a-dunlap
Aug 16, 2018
added a commit
that referenced
this issue
Aug 16, 2018
pdurbin
referenced this issue
Aug 21, 2018
Merged
add Maven profiles: default excludes non-essential unit tests #4896 #4989
This comment has been minimized.
This comment has been minimized.
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
unassigned
pdurbin and
benjamin-martinez
Aug 21, 2018
djbrooke
assigned
pdurbin and
benjamin-martinez
Aug 21, 2018
benjamin-martinez
added
Status: Code Review
Status: QA
and removed
Status: Development
Status: Code Review
labels
Aug 21, 2018
benjamin-martinez
unassigned
pdurbin and
benjamin-martinez
Aug 21, 2018
kcondon
self-assigned this
Aug 21, 2018
added a commit
that referenced
this issue
Aug 21, 2018
kcondon
closed this
Aug 21, 2018
kcondon
removed
the
Status: QA
label
Aug 21, 2018
This comment has been minimized.
This comment has been minimized.
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:
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
reopened this
Aug 23, 2018
pdurbin
added
the
Status: QA
label
Aug 23, 2018
This comment has been minimized.
This comment has been minimized.
@pdurbin Is this related to the actual classifying of the tests or is this a new suggestion for additional checks via Jenkins jobs? |
kcondon
assigned
pdurbin
and unassigned
kcondon
Aug 23, 2018
This comment has been minimized.
This comment has been minimized.
@kcondon I'll explain at standup but the way I think of it is this:
|
matthew-a-dunlap commentedJul 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