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

Check stderr for subprocess call errors #67

Merged
merged 15 commits into from
Feb 15, 2021
Merged

Check stderr for subprocess call errors #67

merged 15 commits into from
Feb 15, 2021

Conversation

jlstevens
Copy link
Contributor

This PR simply moves #59 from a fork to this repo so that I can attempt to fix the tests.

@jlstevens
Copy link
Contributor Author

jlstevens commented Feb 8, 2021

TODO

  • Add comment explaining issue with dodgy return code as detailed in param issue.

Co-authored-by: James A. Bednar <[email protected]>
@jbednar
Copy link
Contributor

jbednar commented Feb 9, 2021

I'm happy to have the actual code change from this PR or #59, but the updates to the tests rely on a slew of other libraries that aren't appropriate to include in the autover codebase, even as optional tests. If it's possible to boil those tests down to a small amount of standard Python, it would be great to include them, but otherwise I'd say the probability that these tests will cause maintenance issues for us over time is much higher than the probability that they will catch actual errors. I'd strongly favor moving towards a design like #66 where released versions are no longer ever calling any subprocesses at all, at which point no such tests should be necessary in any case.

@jlstevens
Copy link
Contributor Author

If it's possible to boil those tests down to a small amount of standard Python, it would be great to include them, but otherwise I'd say the probability that these tests will cause maintenance issues for us over time is much higher than the probability that they will catch actual errors.

I agree and I've updated the PR to just include just the fix (they are still in the history and shown to be passing, thanks @julioasoto!). As you say, once #66 is working this code path should be mostly moot (for release users at least).

@jbednar Think this simplified PR can now be merged?

@jbednar jbednar merged commit 66e9a31 into master Feb 15, 2021
@jbednar jbednar deleted the PR59_test_fix branch February 15, 2021 16:40
@jlstevens jlstevens changed the title Test fixes for PR #59 Check stderr for subprocess call errors Feb 15, 2021
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.

3 participants