Skip to content

Comments

Add TempDir and NewLogFile test utilities#369

Merged
belimawr merged 7 commits intoelastic:mainfrom
belimawr:tempdir-logfile
Dec 5, 2025
Merged

Add TempDir and NewLogFile test utilities#369
belimawr merged 7 commits intoelastic:mainfrom
belimawr:tempdir-logfile

Conversation

@belimawr
Copy link
Contributor

@belimawr belimawr commented Nov 20, 2025

What does this PR do?

This PR adds two helpers for tests:

  • fs.TempDir creates a temporary folder that is kept when tests fails.
  • NewLogFile creates a file that can be used as output for a logger or a a process. Methods to search strings in the file are also provided

Why is it important?

It helps us to keep temporary files and logs generated by tests when the test fails for easier debugging.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works

## Author's Checklist
## Related issues

@belimawr belimawr self-assigned this Nov 20, 2025
@belimawr belimawr added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Nov 20, 2025
@belimawr belimawr marked this pull request as ready for review November 21, 2025 15:19
@belimawr belimawr requested a review from a team as a code owner November 21, 2025 15:19
@belimawr belimawr requested review from andrzej-stencel and leehinman and removed request for a team November 21, 2025 15:19
@pierrehilbert pierrehilbert requested review from rdner and removed request for andrzej-stencel November 21, 2025 16:32
//
// When tests are run with -v, the temporary directory absolute
// path will be logged.
func TempDir(t *testing.T, path ...string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of copying so much of the code that exists in t.TempDir(). Can we use t.TempDir and then just have the cleanup function copy or archive the contents in the case of a failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I like that the temp dir is already created on its final destination, so no matter how wrong the test goes, the files are kept.

Copying/archiving the temp dir after a test failure adds another point of failure and complexity that I'd like to avoid.

I'm not a fan of copying so much of the code that exists in t.TempDir()

I haven't copied much of it, well, the bit I copied was only the part sanitising the test name to create the folder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I like that the temp dir is already created on its final destination, so no matter how wrong the test goes, the files are kept.

Copying/archiving the temp dir after a test failure adds another point of failure and complexity that I'd like to avoid.

I agree, it makes sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a blocker.

but I strongly suggest implementing "removeAll" like the stdlib testing library does. https://cs.opensource.google/go/go/+/refs/tags/go1.25.5:src/testing/testing.go;l=1392-1420

Windows lock files are not released immediately, so even if the code is correct and has released the lock, it can be milliseconds before you can delete the file.

And things like this are why I'm not in favor of reimplementing something that has been thoroughly tested, with lots of edge cases. Especially when it is under active maintenance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting edge case. I can open a follow up PR to implement something like that. Or maybe even do the moving archiving logic. This can be implemented without breaking the API/behaviour.

@belimawr belimawr requested a review from AndersonQ December 3, 2025 17:36
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @belimawr

@belimawr
Copy link
Contributor Author

belimawr commented Dec 3, 2025

@leehinman @kruskall did you folks have a chance to see my replies to your comments?

@belimawr belimawr merged commit 88e330e into elastic:main Dec 5, 2025
5 checks passed
@belimawr belimawr mentioned this pull request Dec 8, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants