util: TempFile improvements #169
Conversation
util/tempfile.go
Outdated
| // tempFile returns errors instead of relying upon T to stop execution, for ease | ||
| // of testing TempFile. | ||
| func tempFile(helper func(), tempDir func() string, settings ...TempFileSetting) (path string, err error) { | ||
| helper() |
There was a problem hiding this comment.
While I understand the point of testing, so returning an error is easier to test
I'm unsure about the arguments pass T methods
Couldn't you make it work with this?
| // tempFile returns errors instead of relying upon T to stop execution, for ease | |
| // of testing TempFile. | |
| func tempFile(helper func(), tempDir func() string, settings ...TempFileSetting) (path string, err error) { | |
| helper() | |
| // tempFile returns errors instead of relying upon T to stop execution, for ease | |
| // of testing TempFile. | |
| func tempFile(t.T, settings ...TempFileSetting) (path string, err error) { | |
| t.Helper() |
Then call t.Tempdir where needed
I have never faced a code that needed to do the kind of thing you did.
Also your failureTracker struct seems more than enough.
The only problem I can see is when the code could call fatal, but you rewrote to return an error.
You could use a panic in failureTracker and catch it in the tests. It would replicate the t.FailNow that exits the current scope.
Anyway, these are random ideas that need to be thought about, and tried.
So I would understand if you reply me: nah, cannot do it
There was a problem hiding this comment.
But I see nothing testing this method, you are only testing the TempFile one right?
There was a problem hiding this comment.
I've tried detecting failures with panic and recover; one downside is that accidental panics get noisy stack traces that way. Before I made this change, I was working up a way to run it in a goroutine and shut it down with runtime.Goexit like testing.T methods do. Both methods require wrapping the code under test in a closure, and overall rather more complicated machinery than what I did here.
I passed the functions instead of T to remove the capability to fail from inside tempDir. I could easily have defined a supertype of T and passed one of those, instead; it doesn't really matter to me either way.
I only test this method through TempFile; since that's its only caller and does almost nothing on its own, I think it's reasonable to treat them from the outside as a single function.
There was a problem hiding this comment.
I just updated tempFile to take an interface argument with a subset of T's methods.
| t.Fatalf("%s: %v", "TempFile", err) | ||
| file, err := os.CreateTemp(*allSettings.dir, allSettings.namePattern) | ||
| if errors.Is(err, fs.ErrNotExist) { | ||
| return "", errors.New("directory does not exist") |
There was a problem hiding this comment.
Usually returning a sentinel could be useful. This way people could catch it
Another solution is to wrap the existing error, allowing people to catch the fs.ErrNotExist, if they have to.
| return "", errors.New("directory does not exist") | |
| return "", fmt.Errorf("directory does not exist: %w", err) |
But these comments might be irrelevant, as you are building a test helper and not a function that people will call and will have to handle its error.
This implements more of @ccoVeille's suggestions on #168:
Pathis nowDirand is better documented.TempFilefails if dir does not exist. I reorganizedTempFileto make it easier to test failures.