search

matthew-brett / delocate

Find and copy needed dynamic libraries into python wheels
BSD 2-Clause "Simplified" License
262 stars 59 forks source link

Replace ScriptRunner with pytest-console-scripts fixtures. #185

Closed HexDecimal closed 7 months ago

HexDecimal commented 1 year ago

This resolves one of the tasks in #125

I'm throwing out scriptrunner.py. Any desired features can be implemented upstream. I've added my own protocols to get around its missing typing.

Updated type-hints and asserts for all the scripts tests. There was more I wanted to do but my changes were beginning to reach into other modules.

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (f639c4d) 94.93% compared to head (9b07f10) 94.93%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #185 +/- ## ======================================= Coverage 94.93% 94.93% ======================================= Files 14 14 Lines 1125 1125 ======================================= Hits 1068 1068 Misses 57 57 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

matthew-brett commented 1 year ago

Looks good - did you check that the script tests fail if the scripts are broken in some way?

HexDecimal commented 1 year ago

Looks good - did you check that the script tests fail if the scripts are broken in some way?

This does what the old script runner was doing but it's maintained upstream and the new script runner is well tested. I've even added my own features and tests (and of course full type-hinting) to the fixture upstream.

Some tests were exact so I'm not sure what you mean by breaking them. Scripts which were expected to fail before still fail as expected as their returncode is checked. There's some changes with the new outputs since this returns strings like subprocess.run instead of lists of strings which were common with the old runner and that will have changed the tests a little.

matthew-brett commented 1 year ago

Sorry - I only meant - did you try breaking the code for the scripts and running the tests to make sure they fail in that circumstance?

HexDecimal commented 1 year ago

did you try breaking the code for the scripts and running the tests to make sure they fail in that circumstance?

I didn't do that. All of the Delocate script tests use MacOS linkage so I can't break them locally. The tests were all broken when the new script runner wasn't returning lists of strings anymore, but that was mostly a process of making the type checker happy rather than waiting on the CI to give me test results. Then after that the tests still pass and test coverage hasn't even changed despite me deleting delocate/tests/scriptrunner.py, so I see that as the new fixture being a perfect replacement for the bundled one.

I did notice that run_command checks the return code and raises on failure so I made sure that check=True was added to the new fixtures, but that was a new feature I just added upstream after starting this PR. There are already scripts which cause a failure and raise exceptions and those were already tested but I went and cleaned up the code more.

If there's anything more specific you want to try then you're in a better position than I am to prod at it.

HexDecimal commented 7 months ago

I've tested "breaking the code for the scripts" and the results are as expected: tests fail with verbose output of the failing command including the error code, stdout, and stderr.

matthew-brett commented 7 months ago

Thanks for this ...

HexDecimal commented 7 months ago

I assume you're too busy to handle these PR's? This PR was blocking me from working on more script related tasks. I'd like to merge 2 other PR's as well (the ones that were requested, which I've self-assigned.)

matthew-brett commented 7 months ago

Yes, sorry - I'm mid-term and very busy at the moment.