pepkit / looper

A job submitter for Portable Encapsulated Projects
http://looper.databio.org
BSD 2-Clause "Simplified" License
20 stars 7 forks source link

Change some tests to no longer use subprocess #386

Closed donaldcampbelljr closed 1 year ago

donaldcampbelljr commented 1 year ago

This addresses: https://github.com/pepkit/looper/issues/353

nsheff commented 1 year ago

out of curiosity, how much of an improvement to test time did this yield?

nsheff commented 1 year ago

this is great! I don't quite see why you're using a list of strings instead of just a dict, the way markmeld does it. Can you explain that?

basically, I think you can do all of this without needing test_args_expansion if you just use the .update function like in markmeld.

donaldcampbelljr commented 1 year ago

This change allows for decreasing test time from 78 secs to 43 secs.

I had issues with passing a dict as test_args, specifically, the arg parser would quit before delivering an args dict to update. I found it was easy to pass a string for the args parser to parse.

Also, there was already a good example using a list of strings for the subprocess method that fit into the testing structure, so I simply used that.

Looking at the markmeld example, there is only one test that uses the approach and it is expected to fail:

def test_cli():
    from markmeld.cli import main

    with pytest.raises(SystemExit):
        main(test_args={"config": "tests/test_data/_markmeld_basic.yaml"})

I'm wondering if the markmeld example is not working exactly as we intend it to but is failing anyway which is what our test wants to see.