-
Notifications
You must be signed in to change notification settings - Fork 10.9k
test(tool-output): stop expressing "unwritable path" as a magic absolute path #4722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
WillemJiang
merged 2 commits into
bytedance:main
from
DaoyuanLi2816:fix/externalize-test-portability
Aug 11, 2026
Merged
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the blocker file just needs to exist as a regular file for
os.makedirsto fail below it — the contents are irrelevant.Path(blocker).touch()(oropen(blocker, "w").close()) would convey that intent a little more directly than writing a literal"placeholder". Totally trivial; feel free to ignore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — applied in 39d0082.
blocker.touch()says "this only needs to exist" without the reader having to work out that the bytes are irrelevant.Re-verified after the change, since the helper is the thing the whole PR rests on:
pytest tests/test_tool_output_budget_middleware.py— 124 passed on Linux, 124 passed on Windows 11except OSError: return Noneremoved from_externalize, bothtest_returns_none_on_invalid_pathandtest_fallback_when_disk_write_failsstill fail (NotADirectoryErroron Linux,FileNotFoundErroron Windows), so the tests still have teeth rather than passing for a new reasonruff check/ruff format --checkcleanThanks for tracing it through
_externalizeyourself rather than taking the description's word for it.