#192 Abstracting SEDStacker Integration Tests and Adding Unit Test Coverage

[eholum]

I worked on this a while ago, and just went through again and cleaned it up/updated it to align with what was in the other PR. It ups unit test coverage for the SED stacker (which is why the build on the other branch is currently broken).

Quick summary:

• Adding unit test coverage for SedStacking.
• Renaming/refactoring integration tests for SED stacking - they still
• require SAMP up so they are still ignored.
• Terminate tests waiting for SAMP connection so they don't hang.
• Cleaning up tabs and replacing with spaces.

Longer summary:

• Logically I didn't change anything. Most of the changes are for cleaning up code - removing tabs, moving class variables to the tops of classes, fixing warnings, etc.
• The largest change is in the SedStackerStacker class - I broke up some of the longer methods into shorter ones, added a constructor for injecting dependencies with stubs which made writing unit tests much easier.
• Pulled the common code out of the some of the integration tests into an abstract test class. I renamed the tests to match out integration test naming conventions, unfortunately they still need SAMP to be up and running to work so they're still ignored.
• Added unit tests for a few of the sed stacking classes.

I verified that the smoketests pass, and that the SedStacker IT tests when sherpa-samp and SAMP are up and running. I'm including Jamie's instructions for that here just in case:

1. start a SAMP hub. To do this with TOPCAT (http://www.star.bris.ac.uk/~mbt/topcat/):
   1. Click on the plug button in the bottom-right corner to disconnect TOPCAT from the SAMP hub
   2. Click the SAMP icon in the task bar of your machine's desktop (top right corner; it's a yellow circle with a black dot in the middle of it) -> Stop Hub
   3. Click on the plug button in the bottom-right corner of the TOPCAT window again. Select "Start external hub." Exit TOPCAT.
   4. Show the SAMP Hub window by clicking on the SAMP icon on the desktop task bar and selecting "Show Hub Window."
2. Start sherpa-samp from the iris development environment
   1. $ source activate iris # or your Iris conda environment name
   2. $ sherpa-samp
   3. You should see "Sherpa" in the SAMP Hub window, under Clients.
3. Uncomment the @Ignore tags before each test. There are 3 of them.

[jbudynk]

Why is all of this moved after the try/catch block?

I know we shouldn't expect any errors with the NED SED (in fact we better not get any errors), but what if, for example, the NED SED doesn't have a SpatialAxis.Coverage.Location.Value?

[This is more for my understanding; I'm not saying this is wrong, just wondering why moving this outside the try/catch block could be better]

[eholum]

If thats a possibility then it sounds like it just needs another null check. I don't want to mess around too deeply with any functionality here, just make things unit testable - so either way's fine for the time being.

As for why though:
I tend to think of throwing/catching of exceptions to be like fishing - the bigger your net the more likely your are to have a problem with sustainability. So when you catch a top-level Exception from a long code block, realize that you're going to catch everything - and that means everything (like extensions of RuntimeExceptions, which can come from anywhere)! So if an OutOfMemoryException was thrown anywhere inside that that code block, it would be neatly wrapped up and sent to caller as a SegmentImporterException, when in reality there's a problem with the JVM. There's no other specific declarations here other than from the code inside that block, so I figure we should let any other unchecked exceptions from this code base bubble up to the caller or the top layer in Iris.

That being said, it's not necessarily a bad thing for the top layer of your application to catch high level exceptions - since it should handle those large issues with user messages, and nice, long stack traces dumped into the logs, but for low level stuff like here I think it's good to be as specific as possible with where exceptions are being thrown and where they are caught, and letting the big fish bubble up.

On the flipside, there are a lot of things that can go wrong, and if you only catch specific exceptions and your application throws a lot of exceptions (SegmentImporterException, SedNoDataException, SampExceptions, etc...) then your application catch blocks might start to get a little long. Fortunately we're now on java 7 and can block them together. It might also be nice to someday have an IrisApplicationException with an error code that can be handled/translated at the top layer. Something to think about.

I like this blog post about exceptions:

http://www.javacodegeeks.com/2013/07/java-exception-handling-tutorial-with-examples-and-best-practices.html

The best practices part has some really nice tips. Also this is old but still relevant:

http://www.javaworld.com/article/2073800/testing-debugging/beware-the-dangers-of-generic-exceptions.html

[jbudynk]

Ah, I see. Ok, makes sense. Thanks for the detailed response!

On Mon, Nov 16, 2015 at 5:07 PM, Erik Holum notifications@github.com
wrote:

In sed-builder/src/main/java/cfa/vo/sed/builder/NEDImporter.java
#195 (comment):

     } catch (Exception ex) {
         throw new SegmentImporterException(ex);
     }

•    if (sed.getNumberOfSegments() > 0) {
  

•        Segment seg = sed.getSegment(0);
  

•        if (seg.createTarget().getPos() == null) {
  

•            if (seg.createChar().createSpatialAxis().createCoverage().getLocation() != null) {
  

•                seg.createTarget().createPos()
  

•                        .setValue(seg.getChar().getSpatialAxis().getCoverage().getLocation().getValue());
  

If thats a possibility then it sounds like it just needs another null
check. I don't want to mess around too deeply with any functionality here,
just make things unit testable - so either way's fine for the time being.

As for why though:
I tend to think of throwing/catching of exceptions to be like fishing -
the bigger your net the more likely your are to have a problem with
sustainability. So when you catch a top-level Exception from a long code
block, realize that you're going to catch everything - and that means
everything (like extensions of RuntimeExceptions, which can come from
anywhere)! So if an OutOfMemoryException was thrown anywhere inside that
that code block, it would be neatly wrapped up and sent to caller as a
SegmentImporterException, when in reality there's a problem with the JVM.
There's no other specific declarations here other than from the code inside
that block, so I figure we should let any other unchecked exceptions from
this code base bubble up to the caller or the top layer in Iris.

That being said, it's not necessarily a bad thing for the top layer of
your application to catch high level exceptions - since it should handle
those large issues with user messages, and nice, long stack traces dumped
into the logs, but for low level stuff like here I think it's good to be as
specific as possible with where exceptions are being thrown and where they
are caught, and letting the big fish bubble up.

On the flipside, there are a lot of things that can go wrong, and if you
only catch specific exceptions and your application throws a lot of
exceptions (SegmentImporterException, SedNoDataException, SampExceptions,
etc...) then your application catch blocks might start to get a little
long. Fortunately we're now on java 7 and can block them together. It might
also be nice to someday have an IrisApplicationException with an error code
that can be handled/translated at the top layer. Something to think about.

I like this blog post about exceptions:

http://www.javacodegeeks.com/2013/07/java-exception-handling-tutorial-with-examples-and-best-practices.html

The best practices part has some really nice tips. Also this is old but
still relevant:

http://www.javaworld.com/article/2073800/testing-debugging/beware-the-dangers-of-generic-exceptions.html

—
Reply to this email directly or view it on GitHub
https://github.com/ChandraCXC/iris/pull/195/files#r44991366.

Jamie A. Budynkiewicz
Smithsonian Astrophysical Observatory
Harvard-Smithsonian Center for Astrophysics
100 Acorn Park Dr. R-377 MS-81
Cambridge, MA 02140

[jbudynk]

This looks good. Nice job refractoring all that code - the abstracted sedstacker tests makes it easier to read the code. And I can confirm that the SedStacker IT tests pass.

Ok to merge.

[jbudynk]

Looks good. Nice work refractoring the tests. We'll have to see how this all works out once we get the SAMP

[olaurino]

I tried to merge locally this branch with the branch setting up the SAMP Hub and starting sherpa-samp (the "samp branch"). The problem is that the code in the samp branch requires the jsamp jar to be available, which happens when the package phase of the iris submodule is run.

The way things are now, the SedStacker tests would not find the SAMP hub running, or sherpa-samp for that matter.

I'll see if I can find some way around it, but the options at the moment seem to be:

• Setup only the iris submodule to run integration tests. I don't like this option because there is not a good reason why all integration tests should be concentrated there.
• Throw away my prototype and start the SAMP hub and the sherpa-samp process through Java. However, since these processes should only be started (and stopped) once, we would end up with an all-encompassing Test Suite, but then we would basically be back to the previous case. One might argue that different suites might start and stop the samp hub and sherpa-samp, but that seems a waste and an unstable process. In any case we should have a single suite for each submodule.

I am still optimist about finding a different solution. We'll see.

[olaurino]

I have some other comments, anyway: I believe it makes sense to include this PR in the integration tests infrastructure package, if you all agree.

[jbudynk]

Ah. I may have to revoke my previous OK comment. I was running mvn clean test instead of mvn clean verify. My tests get hung up in the third redshift test, SedStackerRedshifterIT::testRedshifterNoZ().

There should be an info window that pops up, coming directly from sedstacker (from Python), that will say one of the SEDs isn't shifted. I never see that window pop up, which means one of two things: (1) the tests are working as they should and close the window automatically, or (2) the test is getting hung-up at closing the window. Looking at the SAMP messages from sherpa-samp, I get a successful redshift with an excluded SED. In the case of (1), this is probably as Omar reports it - the SAMP hub isn't shutting down properly.

See https://github.com/jbudynk/sedstacker/blob/master/sedstacker/iris/sed.py#L185 for the info message that sedstacker creates and sherpa-samp sends as a message in Iris.

[olaurino]

In any case if I unignore the SedStacker tests and run mvn clean verify from this branch some of the tests hang and some fail after I ignore the ones hanging. I wouldn't consider this a show-stopper: as noted above there is a different task that is supposed to get those tests unignored.

[olaurino]

Also, it looks like the samp connections from the abstract test class are not being correctly closed. When one of the tests hangs and I open the Hub window I see several SedStacker clients connected to the hub. This can be reproduces both in the vanilla PR and in the temporary merge with the samp branch.

[eholum]

The goal of this task and PR was not to un-ignore the test cases - nor really change any of the base functionality of the tests nor code. Just to break up long methods and move common code to a shared location, then add a unit test for the SedStackerStacker class. If there are other requirements that weren't covered for finishing #192, then we should enumerate and address them here (as long as they are not already covered in a different task). Un-ingoring the abstract tests is already in #172, and it sounds like there's more work that needs to be done with that.

[olaurino]

@eholum I agree. However, this branch was very useful for validating the work with starting up sherpa-samp and the hub.

As I mentioned before I do have some comments related to this PR proper, but at this point I'll keep them for tomorrow morning :)

[eholum]

re: integration testing - I think it's right that each submodule should have it's own internal framework for integration testing that sets up all the expensive threads that the module's integration tests require. We definitely don't want sed-stacker ITs to be dependent on iris setting up SAMP for them, since we should be able to run those tests independently of the parents package. Though whatever setup that needs to be called prior to running a test suite might want to live in iris or the iris-common code, so each submodule can invoke different aspects of it as needed.

[eholum]

But yes, if there are other issues that this PR introduced in the abstract test cases that I'm not aware of, then let's fix them (e.g. incorrectly shutting down the controller, or unknown popups).

[olaurino]

@eholum I mostly agree. However in this case we are talking about setting up (and tearing down) the environment outside of the application scope, just as you ran sherpa-samp and topcat manually.

So, yes:

We definitely don't want sed-stacker ITs to be dependent on iris setting up SAMP for them, since we should be able to run those tests independently of the parents package.

On the other hand, it would be error-prone if each ''@before" method in a submodule started an external python process. Setting up a SAMP hub is not that expensive, however it is even more trivial to do from maven, and less error-prone.

Also, besides iris-common (and maybe test-components and iris-asdc-plugin), there is really not much advantage in having different submodules, and I keep thinking at some point we should reconfigure this aspect.

In any case I've done some progress with the "samp branch", as I'll report on the mailing list. And we should go back to discussing PR #192 in this thread :)

[olaurino]

I think there is no point in holding this further. More work will be done anyway when we unignore the stacker tests (#172) and when we make these tests part of the new integration test infrastructure (#173, #187, #188). Merge it.

[olaurino]

Rebasing: original tip was at b4f2fff