Remove engine binary after tests#1058
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1058 +/- ##
=======================================
Coverage 81.57% 81.57%
=======================================
Files 140 140
Lines 15413 15413
=======================================
Hits 12573 12573
Misses 2840 2840 Continue to review full report at Codecov.
|
| for testfile in glob.glob("test*out") + glob.glob("test*inp"): | ||
| os.remove(testfile) | ||
| engine_binary = os.path.join(engine_dir, "engine") | ||
| os.remove(engine_binary) |
There was a problem hiding this comment.
It looks like the commented code calling make clean was intended to do this same thing. Perhaps that comment can be deleted, or uncommented instead of adding this fix? It looks like it was introduced in this commit and never enabled.
There was a problem hiding this comment.
my vote is to keep as much of our tests inside the same python process (so keeping this code but removing the comments) but I will default to whatever @dwhswenson thinks is best (as the original author of that commit)
There was a problem hiding this comment.
I'd go with the make clean (assuming it works!) My guess is that when I was first iterating on external engines, I felt like compilation was slowing down the tests. So I probably disabled the make clean, since the source files weren't changing at that point.
The advantages of using make clean are:
- Keeps abstraction of the C code compilation separate from the Python.
- Gives a better template/example for more complex use cases (if you were testing against a more complicated compilation that generated a bunch of object files, you'd want your
make cleanto remove all those.ofiles -- that would make more sense than doing that in Python).
dwhswenson
left a comment
There was a problem hiding this comment.
I think the make clean approach makes more sense. See line comment.
| for testfile in glob.glob("test*out") + glob.glob("test*inp"): | ||
| os.remove(testfile) | ||
| engine_binary = os.path.join(engine_dir, "engine") | ||
| os.remove(engine_binary) |
There was a problem hiding this comment.
I'd go with the make clean (assuming it works!) My guess is that when I was first iterating on external engines, I felt like compilation was slowing down the tests. So I probably disabled the make clean, since the source files weren't changing at that point.
The advantages of using make clean are:
- Keeps abstraction of the C code compilation separate from the Python.
- Gives a better template/example for more complex use cases (if you were testing against a more complicated compilation that generated a bunch of object files, you'd want your
make cleanto remove all those.ofiles -- that would make more sense than doing that in Python).
| # proc = psutil.Popen("make clean", cwd=engine_dir, shell=True) | ||
| # proc.wait() | ||
| # Delete compilation files | ||
| proc = psutil.Popen(["make", "clean"], cwd=engine_dir) |
There was a problem hiding this comment.
this has to be a list to be able to run without a shell (which we should default to), otherwise you will get the error:
FileNotFoundError: [Errno 2] No such file or directory: 'make clean'
@bdice identified an issue with our tests leaving files around after running the test. This is bad test design and should be avoided.
This PR fixes the offending test