Repository navigation
bun:test: throw in toHaveLength() and toBeEmpty() for a Blob of unknown size
#46813
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
Open
robobun
wants to merge
1
commit into
main
Choose a base branch
from
robobun/36f80b75/unknown-blob-size-length
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
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
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
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
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
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
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.
🔴 bun:test users whose suites call
.not.toBeEmpty()or.not.toHaveLength(n)on a slice of an existing regular file now get a thrown error where the base passes. The helper at src/runtime/test_runner/expect.rs:1640 treats every NaN from the length probe as an unknown size, butget_size_for_bindings(src/runtime/webcore/Blob.rs:1925) also answersu64::MAXforBun.file(path).slice(a, b)of a regular file that nothing has stat'd yet. Fix: before reporting unknown size, resolve seekability for a file-backed slice whoseseekableis unset (one stat on the store) so a regular-file slice keeps its known length, and only truly unsizable stores (missing path, directory, FIFO) throw. [also at: src/jsc/bindings/bindings.cpp:2993 - Test authors who slice a regular Bun.file() before anything stats it now get a throw from toHaveLength()/toBeEmpty() where the negated forms passed before.]Why this was flagged
A test does
const s = Bun.file("data.bin").slice(0, 10); expect(s).not.toBeEmpty();without reading.sizeon the parent file first.Bun__Blob__getSizeForBindingsgoes toget_size_for_bindingsat src/runtime/webcore/Blob.rs:1913;has_sizeis true for a slice,file.seekableisNoneandmode_seen_by_cloneisNone, so src/runtime/webcore/Blob.rs:1934 returnsu64::MAX. The C++ probe at src/jsc/bindings/bindings.cpp:2993 now returnsPNaNinstead ofDBL_MAX.toBeEmptyat src/runtime/test_runner/expect/toBeEmpty.rs:78 andtoHaveLengthat src/runtime/test_runner/expect/toHaveLength.rs:61 callExpect::throw_nan_length, which finds the value is a Blob and throws "Received value is a Blob whose size is not known: FileRef ("data.bin")". On the base branch the length wasDBL_MAX, so.not.toBeEmpty()and.not.toHaveLength(10)passed (and were correct). ThetoHaveLengthtest block at test/js/bun/test/expect.test.js:2667 only covers slices of a missing path and a directory, not of a regular file, so nothing pins this case.Verification: The C++ probe (src/jsc/bindings/bindings.cpp:2990-2993) now maps that
u64::MAXtoPNaNinstead ofDBL_MAX; toBeEmpty.rs:77-78 and toHaveLength.rs:60-61 then callExpect::throw_nan_length(expect.rs:1640-1649), which, sincefrom_js(value)is a Blob, throws "Received value is a Blob whose size is not known: FileRef (...)" — beforenotis applied.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.
The regression is real: on main
.not.toBeEmpty()passes on such a slice, and with this PR it throws. It is now the first line of the description, as a choice for a maintainer. No code change in this push.I built the suggested stat before I opened the PR, and removed it. A slice that exists before the stat is not cut to the file, so the stat gives the slice its window, not the bytes that are in the file. First use of
Bun.file(path).slice(a, b), release builds:toHaveLength(10)with the statslice(0, 10)of 20 bytesslice(0, 10)of an empty fileslice(0, 10)of 5 bytesslice(30, 40)of 20 bytesThree of the four are silent passes of a wrong length. Main gives the same answers when
file.sizeruns afterslice(), and the right ones when it runs before. The stat also kept its result on the store that a slice shares with itsBun.file(): aBun.file(fd)reportedsize20 after the file grew to 100.A right answer needs the window cut to
st_size. That decides what size a file slice has, and #44504 left that open on purpose: an empty file and a procfs file give the samestat. #45859 and #39763 change the same state. So this PR reports what the Blob layer knows about such a slice, which is nothing.console.logof aResponseover it prints no size for the same reason.A test has two exits:
expect(slice.size).toBe(n), or a read offile.sizebefore the matcher. If a maintainer prefers the stat, it is a small change on top of this one.