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

Change some tests to no longer use subprocess #386

Merged
merged 6 commits into from
Aug 3, 2023
Merged

Conversation

donaldcampbelljr
Copy link
Contributor

This addresses:
#353

@nsheff
Copy link
Contributor

nsheff commented Aug 2, 2023

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

def test_args_expansion(
pth=None, cmd=None, appendix=list(), dry=True
) -> Tuple[bytes, bytes, int]:
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a sentence as to what this is for? I can't infer why we need this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@donaldcampbelljr donaldcampbelljr mentioned this pull request Aug 2, 2023
@nsheff
Copy link
Contributor

nsheff commented Aug 2, 2023

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
Copy link
Contributor Author

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.

@donaldcampbelljr donaldcampbelljr merged commit 819b00c into dev Aug 3, 2023
@donaldcampbelljr donaldcampbelljr deleted the dev_fix_tests branch August 3, 2023 16:14
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.

2 participants