Add unit tests for bonds

[jngrad]

Description of changes:

• handle all edge cases in bond creation/search/serialization
• replace unsafe ast.literal_eval() by equivalent json.loads()
• improve code coverage (partial fix for Improve the code coverage of pyMBE #58)

[pm-blanco]

In search_bond:

• there is an uncovered line in 2549 that only triggers when the switch hard_check is active:

                sys.exit(1) 

In get_bond_length:

• Same issue as before in line 2003

Other than that, it looks very good to me. Thank you for taking the initiative to take care of this @jngrad.

Could you also update the Changelog?

[jngrad]

there is an uncovered line in 2549 that only triggers when the switch hard_check is active

This is meant to be that way. If the original authors intended these errors to be recoverable, they would have opted to raise an exception instead of triggering a fatal error.

I'm planning to open a ticket next week to discuss this topic and propose a more sustainable alternative, because it's not a good design. The Python session crashes without producing a traceback, making it impossible for users to understand what caused the fatal error. It's also untestable: you can try catch a SystemError to intercept sys.exit(1), but the pmb object is now in an indeterminate state and cannot be used safely. In fact, the tearDown() method might call a pmb method to clean up the data.frame, and thus trigger undefined behavior.

As part of this discussion, I'll also touch upon the print statements, which print warnings to stdout instead of stderr, making it more difficult to investigate issues in a slurm job where stdout and stderr and redirected to different files. In addition, print statements don't show the filename and line number to help you diagnose the source of the error, something that logging.warn() does. The advantage of logging is that you can set a severity level for each message, and users can decide globally which severity level should raise an exception. This way, they don't need to pass hard_check to methods. These logs can also be captured in unit tests, which is more convenient than using a contextlib to temporarily redirect stdout to a stream (not to mention the surprise if you capture more than 1 print statement in the same stream).

I'll look into producing a MWE next week, when I have more time to experiment with unittest and logging.

[pm-blanco]

there is an uncovered line in 2549 that only triggers when the switch hard_check is active

This is meant to be that way. If the original authors intended these errors to be recoverable, they would have opted to raise an exception instead of triggering a fatal error.

I can tell you that the original author simply did not think too much about it when wrote those lines ;) I agree that we should replace this logic with a better code design, that is also why I raised this issue. What you propose sounds like a much more cleaner solution, I like it. Let's then leave those lines uncovered by now and add coverage once we refactor the error handling.