Skip to content

fix: handle filepath.Walk errors in Analyze#197

Merged
depado merged 4 commits into
depado:mainfrom
shv-ng:fix/analyze-error-handling
Nov 8, 2025
Merged

fix: handle filepath.Walk errors in Analyze#197
depado merged 4 commits into
depado:mainfrom
shv-ng:fix/analyze-error-handling

Conversation

@shv-ng

@shv-ng shv-ng commented Nov 6, 2025

Copy link
Copy Markdown
Contributor

Summary

This PR improves error handling in the Analyze function within the renderer package. It addresses an issue where filepath.Walk silently ignored errors when encountering unreadable files or directories (e.g., due to permission restrictions).

What was changed

  • Updated the Analyze function to check and return errors from filepath.Walk.
  • This change causes permission errors to surface instead of being ignored.
  • Added a test case to verify that unreadable files trigger the appropriate error.

renderer.Analyze accessing a file/dir with permission restricted 0000

Before:

> go test ./renderer -v
=== RUN   TestAnalyze_WithUnreadableFile
2025/11/06 14:46:53 » test -
--- PASS: TestAnalyze_WithUnreadableFile (0.00s)
PASS
ok      github.com/depado/quokka/renderer       0.006s

After error handling:

>  go test ./renderer -v
=== RUN   TestAnalyze_WithUnreadableFile
2025/11/06 14:47:48 » test -
2025/11/06 14:47:48 » Couldn't read filesystem: open /tmp/TestAnalyze_WithUnreadableFile2830109613/001/restricted: permission denied
FAIL    github.com/depado/quokka/renderer       0.007s
FAIL

@depado

depado commented Nov 8, 2025

Copy link
Copy Markdown
Owner

Good catch! Thanks for that!
I have changed the way Analyze works by returning a proper error so it can actually be tested, before that it couldn't (my bad). In its current state your test will always fail, could you:

  • Make it so the test cleans up the directory after it runs so it doesn't clutter the temporary dir
  • Checking for errors in the test when creating files (using if err != nil { t.Error(...) }
  • Checking that Analyze actually returns the expected error (inverted logic if err == nil { t.Error(...) }
  • You can ignore errors in the defer check by placing a //nolint: errcheck next to the line you wan to ignore error checking for (for example in your defer statement)

Let me know if you need help with that!

@shv-ng

shv-ng commented Nov 8, 2025

Copy link
Copy Markdown
Contributor Author

Updated the test as requested:

  • Added error checking for file/directory creation
  • Added inverted logic to expect error from Analyze
  • Added nolint comment for defer cleanup

Thanks for the feedback!

@depado depado merged commit ff1e81f into depado:main Nov 8, 2025
5 checks passed
@depado

depado commented Nov 8, 2025

Copy link
Copy Markdown
Owner

Thanks for your PR!
Your fix is in the v1.4.3 that is being published 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants