search

thasso / pyjip

JIP Pipeline System
http://pyjip.readthedocs.org
Other
19 stars 8 forks source link

New: Check for presence of cluster commands in PATH #49

Closed Poshi closed 10 years ago

Poshi commented 10 years ago

Two comments: I've not been able to push directly, so I forked and push-requested. And second comment: it has not been tested yet!

Poshi commented 10 years ago

I saw that the tests fail. I put the checks in the initializations of the classes, which is the cause that they fail in the tests, because during the tests, the proper binaries are not in place. One option to pass the tests is to create fake executables, another one is to move the exec test to the methods that use those executables.

thasso commented 10 years ago

Hmm, it would be good to get the tests pass before we merge this in. I also noticed that the exception raised in the test is in fact ClusterImplementationError, but it should be the ExecutableNotFoundError if its really due to missing executables, isn't it? And yeah, either fake executables in the test setup or we change the test in a way that it expects the exception and in fact fails if the exception is not triggered.

Also one small comment for the second commit (382f70d). I completely agree that the json parsing should be handle better, but I think we should still raise an exception here so the execution stops. Maybe one that is catched in jip_main and only the message is printed, for example jip.ValidationError

Poshi commented 10 years ago

OK, I added test cases for ExecutableNotFoundError, to make sure than when they are not in PATH this exceptions is triggered. It was appearing as ClusterImplementationError because the code was capturing any exception and raising this one with a human explanation. I also modified existing test cases to create a temporary folder before execution of the code of interest, populating them with all possible binaries and modifying PATH accordingly, and removing all this files and reverting PATH back to its original state once the execution has finished. Now the tests passes :-) Later I will work on json handling to meet your requirements.

Poshi commented 10 years ago

OK, I switched from ValueError to ValidationError and filled in the params. Anyways, no exception is considered: at some point, Exception is just ignored and execution continues. The error log messages are still printed, so user is warned, somehow.