separated integration tests #531

[codeallthethingz]

gradle test now runs all tests except those tagged with Category(IntegrationTest.class) gradle check runs all tests as before

mvn test now runs all tests except those tagged with Category(IntegrationTest.class) mvn integration-test / mvn verify runs all tests

gradle test now runs in ~16 seconds.

[rhyolight]

I think he's signed the CL now.

[rhyolight]

I like splitting tests up like this. 👍

[cogmission]

Hi @willwandering,

Does grade test also run the benchmarks? There is a flag which is documented on the project landing page here which documents a method created to avoid running them:

gradle -Pskipbench check

Also the longest running tests are in the Network package (this may be slightly inaccurate as it is from memory and I've been concentrating on another project lately) - anyway, it isn't an integration test.

Honestly, I'm not convinced of the value of splitting up tests because I fear it could lead to undiscovered impacts by newly developed code? There already is a facility (with gradle and I documented it in our build.gradle file), for running individual tests, which are handy for fast code-compile-test cycles...

What do you think?

(And Welcome Aboard!) :-)

[codeallthethingz]

Thanks for the welcome, I'm super happy to contribute to this revolutionary technology!

I'll make my argument here, but I'm happy to get on a call / hangouts to discuss as, holy moly, it was long, and might come across as a bit preachy. :)

My overarching goal is to make all developers on htm.java be able to build software faster. Please don't think of this separation of concerns as a purely religious in nature, all of the below is practical and based on my experience of managing multiple teams of developers building a large distributed cloud platform with stringent uptime requirements. I think the htm.java code base has great coverage and is well modularized and my goal is to make it, hopefully, even more awesome.

General rules that I subscribe to:

1. Unit: You should have most of your test coverage in fast running unit tests which is true in htm.java but for a few exceptions.
   • Generally you should be runninrg all the unit tests when you change or add code, not just the unit test that you think pertains to the code you're changing. It's valuable to the speed of development to get feedback about something that broke in a strange place in the code base as soon as possible so you can understand the scope of impact.
2. Integration: You should have integration tests that test the functional connections between the code and there should be fewer of these and they typically run more slowly.
3. Regression: You should have regression tests that test things such as performance and end to end behaviours of the system - they take a lot longer to run than integration tests and you generally have less of these than integration tests.

It's crucially important that your unit tests are fast. If they take a long time to run, developers will get out of the habit of running all of them frequently. Running code coverage tools before you start coding will be onerous if your unit tests take a long time and its very valuable to see, before you start to code, if you're changing an area of code that has no coverage as you're at high risk of regression that makes it into deployed code in these gaps.

You raise a very valid concern that the integration tests are important check and balance before code gets merged into master. The developer should generally run these after they have finished segments of the work before everything is commited to a branch. Definitely these tests should be run as part of the merge pre-checks that github provides and with a bit of modification to the travis-ci integration this can be baked in.

I did notice the if statement in the build script to exclude the performance tests. This leads to a few minor-ish problems:

• A more complex build script than necessary.
• You have to duplicate the logic in both the pom file and the gradle file which can lead to semantic drift and problems for ramping up new developers. In the future weird build bugs that are tricky to track down may come up.
• It uncouples the type of test from the test code. To write an integration test you shouldn't have to know about the build script. With the annotation approach it's clear to the developer from just looking at the code how to make this an integraiton test and stop it from being run when you run unit tests only.
• New types of test require reinvention of build script code for solutions to this well solved problem.

If the unit tests, integration tests and regression tests are all separated, you can run these groups of tests in paralell on your build server. This gives the advantages:

• If the unit tests fail the entire build fails as quickly as possible and all development (generally) should stop while this is fixed. If intergration tests are mixed in with unit tests you might have to wait tens of minutes for a build to fail.
• If the integration/regression tests fail they do so more quickly as the build server started them at the same time as the unit tests.
• In the (maybe) distant future, as the number of pull requests increase per day as more developers get involved, the chance of the merge to master causing a unit test / integration test failure increase and making these failures as fast and painless as possible is critical to long term health of a code base.

Maybe the performance tests should be moved annotated regression, and the slow unit tests as integration. Currently I only made the one distinction but it might make more sense to start with all three types of test.

[rhyolight]

What do you think @cogmission?

[cogmission]

@rhyolight Will seems to have a lot of experience with this, and I'm in favor of it. I apologize @willwandering, I'm swamped right now and haven't had time to review - I'll get to it a.s.a.p.

The only thing I'm concerned about immediately is that the Network package's tests aren't integration tests - they're unit - but maybe an exception can be made there? Also they are the longest running tests. I just haven't had a chance to adequately consider it...

[codeallthethingz]

@cogmission no worries about the delay I'm on holiday in Hawaii 🌴 🏄

The process I tackle slow unit tests with:

1. Can you make the unit tests go faster?
   • by mocking dependencies or exposing and mocking parts of the object under question?
   • Is the object under test is complex to instantiate and therefore using factory methods that take a long time to run, can the factory expose just the parts of the object that are needed? (this indicates that these tests are in fact integration tests not unit tests)
2. If not 1) then we might consider marking them as integration tests to mark those as to be run in the integration phase.

I'd prefer always going down the path of making slow unit tests fast and then moving slow unit tests into the integration phase so developers don't stop running the unit tests.

I'm happy to have a look at the Network tests to see if they can be sped up.

[cogmission]

Hi @willwandering,

Are you still there? I apologize for the delay - I've been in a deep dive on another project (for my day job) and totally forgot about this! I really can't believe I let this slide so long - I'm very sorry! I have had a family tragedy and have been concentrating on spending time at with my immediate family at home and have not been thinking much about this, also due to the fact that Numenta was "switching gears" to "research mode" and new development on NuPIC was severely ramping down. I just haven't had my attention on it like I should have.

Let me know if you're still interested, and I'll check out your code and give it a go?

[cogmission]

Hi @willwandering,

I haven't forgotten about this, I'm still in an intense dev cycle trying to get this major feature done in another project with Cortical.io - please bear with me... I just don't have the cycles to analyze this and mull over it right now... Again, please bear with me because I want to give this a lot of consideration.

[cogmission]

Hi @codeallthethingz,

Ok, after reviewing your proposed changes, I came up with a compromise for the NetworkTest. My problem was that I didn't want to classify the NetworkTest as "integration" and encourage building without invoking this test because it is crucial that discrepancies which cause failures in the Network API be caught and dealt with as early as possible. So, as a solution I created a new test case (NetworkIntegrationTest), and moved the time-consuming tests to that class and added the IntegrationTest annotation to that and removed it from the regular NetworkTest test case.

This way the crucial parts (which aren't the longer running parts thankfully) are run during the regular test/dev cycle.

I also removed the IntegrationTest annotation from the FieldMetaTypeTest because it runs very quickly and it isn't an integration test. Same with the CoordinateEncoderTest and the GeospatialCoordinateEncoderTest.

If you are still around and if you would like me to, I can push the changes to your PR, but please let me know today so I can go ahead and get this out of the way - otherwise I will check this in under a different PR (I will credit you there though of course!).

Again I'm sorry for the ridiculous delay and I hope you'll continue contributing great work to the project!

Thanks, David