chore: Prevent test suite artifact creation in work directory #6438
Conversation
Also remove a few other stale test assets while I'm here.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6438 +/- ##
==========================================
+ Coverage 88.78% 88.83% +0.04%
==========================================
Files 296 304 +8
Lines 41320 41505 +185
==========================================
+ Hits 36687 36871 +184
- Misses 4633 4634 +1 ☔ View full report in Codecov by Sentry. |
|
|
There are a ton of directories that are being created in |
|
@rjsparks I think I made all the changes you requested? |
|
Yes, I'm holding this for more work post the feat/rfc merge. |
|
This didn't get returned to post feat/rfc. We should review it fresh to see if it's still the right thing to do. |
|
I left a couple comments - cleaning up after the tests, even in |
|
Hopefully not stepping on @larseggert's toes, but just pushed changes that address my concerns. I think it's ready for @rjsparks to re-review? |
|
No toes in the area :-) I won't get to this any time soon, so please do what you need to! |
There was a problem hiding this comment.
I am a little nervous at the "clean up on exit" vs "clean up as you go" approach as it adds to the recovery for the developer if the process goes away via kill -9 or the like, but am willing to try it.
Just one question about why we're trading a file that I think we should be keeping for PIL.
| img = Image.new('RGB', (200, 200)) | ||
| img.save(photodst) |
There was a problem hiding this comment.
remind me why we're not just making a copy of the profile-default.jpg again?
(I'm sensitive to the PIL import - it's large)
There was a problem hiding this comment.
I think it's just avoiding a file that (as far as I can tell) exists only for the tests.
I don't have an opinion one way or the other, but maybe there's a lighter weight library for generating an empty image for testing.
There was a problem hiding this comment.
There's PyPNG - much smaller it seems, but requires less clearly intentioned code to generate a blank image and is somewhat slower on a per-image basis. The import of PIL on my system looks to be a few tens of ms - and I think we probably only hit that once per test run. (Though I haven't actually done a profile to see if the import is being exercised more than once; not sure how to do that)
There was a problem hiding this comment.
I suspect there's a valid, small, literal we can use as a PNG, but not going to slow this down over it.
I think we should, however, spend time scrubbing our full dependency set and seeing if we can make it smaller, and PIL is one of the things I'm wondering if we can scrub completely away.
There was a problem hiding this comment.
We use PIL for resizing images in Person.photo. If we do away with that (mis?)feature, then we can probably ditch even needing to fake an image.
Checked and PyPNG is a < 40kb python file. It can read and write PNGs, so if we do need to create one for some reason, it's a pretty minimal bit of baggage.
I'm sympathetic. The The cleanup-at-exit does worry me though because I'd much rather create the temp dir separately for each test to maintain isolation. Before adding the |
Also remove a few other stale test assets while I'm here.
Fixes #4020
Fixes #3520