Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PAML: added testing for the presence of the PAML binaries #17

Closed
wants to merge 7 commits into from
Closed

PAML: added testing for the presence of the PAML binaries #17

wants to merge 7 commits into from

Conversation

brandoninvergo
Copy link
Contributor

I added tests for the PAML binaries at the head of each of the test files (test_PAML_[codeml, baseml, yn00].py). Since the programs don't accept commandline arguments other than the control file, they can do nothing other than actually process a control file or hang around waiting for user input. Instead, I implemented something like the unix which command. Windows users are instructed to move the executables to a folder and then to add that folder to their system path. However, in the case of Windows, I also add some Program Files paths to search (like in test_muscle_tool.py) just in case they chose to do things their own way.

@brandoninvergo
Copy link
Contributor Author

Hang on, hold the pull for a moment....

@brandoninvergo
Copy link
Contributor Author

Ok, I forgot to include adding the '.exe' for Windows. It's in there now. I've run the tests on Windows 7 with Python 2.7 and they pass.

All set for the pull.

(what would a pull request be from me without at least one "hang on..." (said with shame))

@peterjc
Copy link
Member

peterjc commented Aug 16, 2011

Hi Brandon,

The executable detection stuff looks fine, but do you actually have any new tests which invoke the binary?

Also I'd prefer to keep test_PAML_yn00.py as they were (no binary required), and add a new test_PAML_tools.py (or set of test files one for each tool if you prefer) which actually use the binary. The point being currently Biopython's unit test skipping for external dependencies is at the file level - all or nothing. Apologies if that wasn't clear previously, but it should be straightforward to move the new functions into a new file.

Peter

@brandoninvergo
Copy link
Contributor Author

I opted against actually invoking the binary because in most cases the program hangs, waiting for user input. I just figured out that if you pass it a text file which doesn't contain valid configuration options, it just throws an error. True to PAML form, each one does something different:

`$ codeml test.txt

Error: err: option file. add space around the equal sign?.
$ yn00 test.txt
YN00 in paml version 4.3, August 2009

Reading options from test.txt..

Error: option file..
$ baseml test.txt
BASEML in paml version 4.3, August 2009

Error: option file..
`
So, we have a nice way to get them to immediately exit, but we're back at the dummy file thing. I was a bit confused by your /dev/null suggestion in the email from the 11th. Have any preferences/suggestions?

Moving the testing for the presence of the executables to another file is no problem.

@brandoninvergo
Copy link
Contributor Author

I can write some tests that do the quickest possible models in paml...they should clock in under a minute each. I'll do a basic test to make sure the output is being parsed properly. We already have all the files necessary in the Tests/PAML directory. I'll just have to make a few changes. Sound good?

@peterjc
Copy link
Member

peterjc commented Aug 16, 2011

Try to get a fast (but trivial) control file working for each tool, and that should do nicely.

P.S. You can remove the binary dependency testing from the test_PAML_yn00.py etc now - we want those tests to run all the time even if the PAML binaries are not installed locally.

@brandoninvergo
Copy link
Contributor Author

Done!

@peterjc
Copy link
Member

peterjc commented Aug 17, 2011

Close enough - I cherry-picked the relevant changes and then did a little polishing.

If you could checkout the master branch and retest locally that would be great - I'm just triggering the buildslaves now...

Thanks,

Peter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants