Parallel tests

[ajs6f]

Parallelize Alpaca tests: (Islandora/documentation#712)

What does this Pull Request do?

Parallelizes the tests in alpaca/islandora-indexing-fcrepo by route.

What's new?

Splits tests up into classes by route being tested and runs them in parallel.

An additional commit is included showing a possible shortening of test setup patterns. If it looks attractive, I can use it more widely.

How should this be tested?

This PR should not change the functional behavior of Alpaca at all. It should speed up build time by some small amount.

Interested parties

@Islandora-CLAW/committers

[codecov]

Codecov Report

Merging #44 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #44   +/-   ##
=========================================
  Coverage     91.74%   91.74%           
  Complexity       58       58           
=========================================
  Files             8        8           
  Lines           666      666           
  Branches          3        3           
=========================================
  Hits            611      611           
  Misses           52       52           
  Partials          3        3

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea1d9aa...24c8a2a. Read the comment docs.

[dannylamb]

@ajs6f After pulling this down and running a ./gradlew clean build I'm getting java.lang.IllegalStateException: ProducerTemplate has not been started for every islandora-indexing-fcrepo test.

Travis is passing though, so am I missing something?

[ajs6f]

Hm, weird. I never saw anything like that. What's your setup? (OS, JDK, etc)

[acoburn]

@ajs6f it is my understanding that, if you use Camel's AdviceWith mechanism, you need to manually run Context::start in the route tests. See here for more information: http://camel.apache.org/advicewith.html In particular:

From Camel 2.9: it's recommended to override the isUseAdviceWith() method and return true to tell Camel you are using advice with in your unit tests. Then after you have done the adviceWith(), then you must start CamelContext manually.

[ajs6f]

@acoburn What is odd is that I specifically took the context.start()s out, because the context is getting started in the (Camel) superclasses and because the adviceWith calls on the RouteDefinitions restart those routes (which is actually annoying, but they do). So I don't understand why this works on my box and Travis but not on @dannylamb's.

I'd like to get to the bottom of this, but I'm actually not at work today, so I won't be doing much with this today.

[dannylamb]

@acoburn @ajs6f That's interesting. And it's certainly not a pressing issue.

FYI I'm on ubuntu 16.04 with openjdk version 1.8.0_131. I also wiped my repo and re-cloned just to make sure there wasn't any caching shenanigans and hit the errors again.

[acoburn]

@ajs6f it has been a while since I really dug into the details of how camel does this -- but as I recall, the camel superclasses do start the context, but they do so before the routes are "patched" with adviceWith. And that's why one needs to call context.start() again after these changes, though (IIRC) I've also seen variability in the specific behaviors of test routes w/r/t manually calling context.start(). I'll try running this on my machine (JDK 9)

[acoburn]

JDK 8 seems to work fine for me, but JDK 9 doesn't work so well with the tests (lots of NoClassDefFoundErrors, which isn't entirely surprising). At some point, it might make sense to enable JDK 9 tests on travis, given that JDK 9 will be released today. Here is an example -- you can even configure it so that Travis permits test failures. Also, (and this is orthogonal to the discussion here), you may also want to enable Windows-based tests using AppVeyor: it's just another angle to test your code in a different environment. See here for an example.

[ajs6f]

@acoburn I think those are good ideas-- can you open two tickets for them, because I have two toddlers and by the time I get to the end of this sentence I will have already forgotten what the two good ideas were. What two good ideas?

[acoburn]

@ajs6f thanks for the nudge. Here are the tickets: Islandora/documentation#715 and Islandora/documentation#714

[ajs6f]

Okay, so I'm going to add in a bag of context.starts() to see if that helps @dannylamb . Before I do that, are there opinions about the second commit? Think optometry-- is it better or worse?

[dannylamb]

@ajs6f I think the parallelism is what's important here. The utility functions are great, but I wouldn't get too wrapped up in improvements to these tests. Now that Api-X installs and claw-playbook is settling down, I can finally issue PRs for Islandora/documentation#640, including https://github.com/dannylamb/Alpaca/tree/issue-640, which significantly reduces the scope of islandora-indexing-fcrepo and its tests.

[ajs6f]

K, for expediency I'll back out that last commit, put in the context.starts(), and we can try it again.

[ajs6f]

@dannylamb That should do it, I hope. Give 'er a try!

[dannylamb]

@ajs6f Travis seemed to hiccup there. I restarted it.

Getting 1 test failure now due to a java.util.zip.ZipException, so I'm going to retry those too and see what happens. Seems weird just one of those would pop.

[ajs6f]

Weird, yeah. Every once in a while I've seen a ZipException from a corrupted JAR file.

[dannylamb]

Seems like we're running more tests now? islandora-indexing-fcrepo just ran 97 tests and took 18 minutes to complete locally. I don't recall the exact amount, but I thought the number of tests was more like 50, not 100.

[ajs6f]

TBH, I don't understand how Gradle is counting tests at all-- @Test annotations?

[acoburn]

@ajs6f yes, it uses the @Test annotation

[dannylamb]

daniel@daniel-Latitude-3560:~/Code/Java/Alpaca$ grep "@Test" islandora-indexing-fcrepo/src/test/java/ca/islandora/alpaca/indexing/fcrepo/*.java | wc -l
49

Looks like we're almost doubling up?

[ajs6f]

Travis seems to have failed at git checkout?

[ajs6f]

@dannylamb @acoburn I have no idea about the "doubled" tests. Let me check and see if my test suite setup is running them, and then they are running by themselves. I should be able to fix that.

[acoburn]

@ajs6f you may also find some clues if you look at the Gradle test report.

[dannylamb]

@acoburn++ I just dug through mine and can confirm the tests are getting run twice.

[ajs6f]

k, I'll rework the test execution to fix that.

[ajs6f]

Now I'm using Gradle's forking ability to run in parallel. Let's see how that goes.

[dannylamb]

@ajs6f I'm running it now and it behaves like it's running in parallel. It's exhibiting long pauses followed by 4-5 successes at once.

[dannylamb]

And is running exactly 49 tests. Think you got it this time.

[ajs6f]

Good, although that's what it was doing before for me, so now

[dannylamb]

Indeed