-
Notifications
You must be signed in to change notification settings - Fork 5.1k
bun test: do not create an empty .snap file when CI refuses a new snapshot #41719
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -103,9 +103,32 @@ impl InlineSnapshotToWrite { | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// The `.snap` file of the test file that is running. `write_snapshot_file` | ||||||||||||||||||||||||
| /// creates or replaces it on disk once entries were added. | ||||||||||||||||||||||||
| pub struct File { | ||||||||||||||||||||||||
| pub(crate) id: FileId, | ||||||||||||||||||||||||
| pub(crate) file: bun_sys::File, | ||||||||||||||||||||||||
| /// Directory of the test file, with a trailing slash. Borrowed from the | ||||||||||||||||||||||||
| /// runner's `File::source.path`, a `Path<'static>`. | ||||||||||||||||||||||||
| test_dir: &'static [u8], | ||||||||||||||||||||||||
| /// NUL-terminated path of the `.snap` file: `test_dir`, `__snapshots__/`, the file name. | ||||||||||||||||||||||||
| path: Vec<u8>, | ||||||||||||||||||||||||
| /// `file_buf` holds entries the file on disk does not have. | ||||||||||||||||||||||||
| dirty: bool, | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| impl File { | ||||||||||||||||||||||||
| fn path_z(&self) -> &ZStr { | ||||||||||||||||||||||||
| ZStr::from_slice_with_nul(&self.path) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// The `__snapshots__` directory, NUL-terminated. | ||||||||||||||||||||||||
| fn snapshots_dir_z(&self) -> Vec<u8> { | ||||||||||||||||||||||||
| let len = self.test_dir.len() + Snapshots::SNAPSHOTS_DIR_NAME.len(); | ||||||||||||||||||||||||
| let mut dir = Vec::with_capacity(len + 1); | ||||||||||||||||||||||||
| dir.extend_from_slice(&self.path[..len]); | ||||||||||||||||||||||||
| dir.push(0); | ||||||||||||||||||||||||
| dir | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| impl Snapshots { | ||||||||||||||||||||||||
|
|
@@ -145,19 +168,8 @@ impl Snapshots { | |||||||||||||||||||||||
| .bun_test() | ||||||||||||||||||||||||
| .ok_or(crate::Error::SnapshotFailed)?; | ||||||||||||||||||||||||
| let bun_test = buntest_strong.get(); | ||||||||||||||||||||||||
| match self.get_snapshot_file(bun_test.file_id)? { | ||||||||||||||||||||||||
| bun_sys::Result::Ok(()) => {} | ||||||||||||||||||||||||
| bun_sys::Result::Err(err) => { | ||||||||||||||||||||||||
| // `bun_sys::Tag` is a newtype-struct with assoc consts (lowercase), | ||||||||||||||||||||||||
| // not an enum — match arms require structural-eq; use if-chain instead. | ||||||||||||||||||||||||
| return Err(if err.syscall == bun_sys::Tag::mkdir { | ||||||||||||||||||||||||
| crate::Error::FailedToMakeSnapshotDirectory | ||||||||||||||||||||||||
| } else if err.syscall == bun_sys::Tag::open { | ||||||||||||||||||||||||
| crate::Error::FailedToOpenSnapshotFile | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| crate::Error::SnapshotFailed | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if self.get_snapshot_file(bun_test.file_id)?.is_err() { | ||||||||||||||||||||||||
| return Err(crate::Error::FailedToOpenSnapshotFile); | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): This change removes the only producer of Extended reasoning...Before this diff, Verification: nit — The diff removes the only producer of |
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let (name, counter) = self.add_count(expect, hint)?; | ||||||||||||||||||||||||
|
|
@@ -216,6 +228,9 @@ impl Snapshots { | |||||||||||||||||||||||
| .map_err(|_| crate::Error::WriteError)?; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.added += 1; | ||||||||||||||||||||||||
| if let Some(file) = self._current_file.as_mut() { | ||||||||||||||||||||||||
| file.dirty = true; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| self.values | ||||||||||||||||||||||||
| .insert(name_hash, Box::<[u8]>::from(target_value)); | ||||||||||||||||||||||||
| Ok(None) | ||||||||||||||||||||||||
|
|
@@ -237,38 +252,8 @@ impl Snapshots { | |||||||||||||||||||||||
| let arena = bun_alloc::Arena::new(); | ||||||||||||||||||||||||
| let mut temp_log = bun_ast::Log::init(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // do NOT call `Jest::runner()` here — it hands out an exclusive ref to the global TestRunner, | ||||||||||||||||||||||||
| // and `self: &mut Snapshots` is a live borrow of that same TestRunner's `.snapshots` | ||||||||||||||||||||||||
| // field. Retagging the whole TestRunner would invalidate `self` under Stacked Borrows. | ||||||||||||||||||||||||
| // Project the disjoint `.files` sibling through the raw `RUNNER` pointer instead. | ||||||||||||||||||||||||
| // SAFETY: single-threaded JS VM; RUNNER is set before any Snapshots method runs | ||||||||||||||||||||||||
| // (Snapshots is a field of TestRunner). Raw-pointer place projection touches only | ||||||||||||||||||||||||
| // `.files` bytes, disjoint from `&mut self`. | ||||||||||||||||||||||||
| let test_file_source = unsafe { | ||||||||||||||||||||||||
| let p = Jest::RUNNER.read().expect("Jest runner not set").as_ptr(); | ||||||||||||||||||||||||
| &(*p).files.items_source()[file.id as usize] | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| let name = test_file_source.path.name(); | ||||||||||||||||||||||||
| let test_filename = name.filename; | ||||||||||||||||||||||||
| let dir_path = name.dir_with_trailing_slash(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let mut snapshot_file_path_buf = bun_paths::path_buffer_pool::get(); | ||||||||||||||||||||||||
| let buf = snapshot_file_path_buf.0.as_mut_slice(); | ||||||||||||||||||||||||
| let mut pos = 0usize; | ||||||||||||||||||||||||
| buf[pos..pos + dir_path.len()].copy_from_slice(dir_path); | ||||||||||||||||||||||||
| pos += dir_path.len(); | ||||||||||||||||||||||||
| buf[pos..pos + Self::SNAPSHOTS_DIR_NAME.len()].copy_from_slice(Self::SNAPSHOTS_DIR_NAME); | ||||||||||||||||||||||||
| pos += Self::SNAPSHOTS_DIR_NAME.len(); | ||||||||||||||||||||||||
| buf[pos..pos + test_filename.len()].copy_from_slice(test_filename); | ||||||||||||||||||||||||
| pos += test_filename.len(); | ||||||||||||||||||||||||
| buf[pos..pos + b".snap".len()].copy_from_slice(b".snap"); | ||||||||||||||||||||||||
| pos += b".snap".len(); | ||||||||||||||||||||||||
| buf[pos] = 0; | ||||||||||||||||||||||||
| // SAFETY: buf[pos] == 0 written above | ||||||||||||||||||||||||
| let snapshot_file_path = ZStr::from_buf(&buf[..], pos); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let source = bun_ast::Source::init_path_string( | ||||||||||||||||||||||||
| snapshot_file_path.as_bytes(), | ||||||||||||||||||||||||
| file.path_z().as_bytes(), | ||||||||||||||||||||||||
| self.file_buf.as_slice(), | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -345,19 +330,50 @@ impl Snapshots { | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| pub(crate) fn write_snapshot_file(&mut self) -> Result<(), Error> { | ||||||||||||||||||||||||
| if let Some(file) = self._current_file.take() { | ||||||||||||||||||||||||
| file.file | ||||||||||||||||||||||||
| .write_all(&self.file_buf) | ||||||||||||||||||||||||
| .map_err(|_| crate::Error::FailedToWriteSnapshotFile)?; | ||||||||||||||||||||||||
| let _ = file.file.close(); | ||||||||||||||||||||||||
| self.file_buf.clear(); | ||||||||||||||||||||||||
| self.file_buf.shrink_to_fit(); | ||||||||||||||||||||||||
| let Some(file) = self._current_file.take() else { | ||||||||||||||||||||||||
| return Ok(()); | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| let result = if file.dirty { | ||||||||||||||||||||||||
| self.create_snapshot_file(&file) | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| self.file_buf.clear(); | ||||||||||||||||||||||||
| self.file_buf.shrink_to_fit(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.values.clear(); | ||||||||||||||||||||||||
| self.values.clear(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.counts.clear(); | ||||||||||||||||||||||||
| self.counts.clear(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| result.map_err(|err| { | ||||||||||||||||||||||||
| bun_output::err( | ||||||||||||||||||||||||
| err, | ||||||||||||||||||||||||
| "Failed to write snapshot file: {}", | ||||||||||||||||||||||||
| (bstr::BStr::new(file.path_z().as_bytes()),), | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| crate::Error::FailedToWriteSnapshotFile | ||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Creates `__snapshots__/` if needed, then replaces the `.snap` file with `file_buf`. | ||||||||||||||||||||||||
| fn create_snapshot_file(&mut self, file: &File) -> bun_sys::Result<()> { | ||||||||||||||||||||||||
| let cached_dir = self.snapshot_dir_path; | ||||||||||||||||||||||||
| if cached_dir.is_none() || !strings::eql_long(file.test_dir, cached_dir.unwrap(), true) { | ||||||||||||||||||||||||
| let dir = file.snapshots_dir_z(); | ||||||||||||||||||||||||
| match bun_sys::mkdir(ZStr::from_slice_with_nul(&dir), 0o777) { | ||||||||||||||||||||||||
| bun_sys::Result::Ok(()) => {} | ||||||||||||||||||||||||
| bun_sys::Result::Err(err) if err.get_errno() == bun_sys::Errno::EEXIST => {} | ||||||||||||||||||||||||
| bun_sys::Result::Err(err) => return Err(err), | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| self.snapshot_dir_path = Some(file.test_dir); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let out = bun_sys::File::open( | ||||||||||||||||||||||||
| file.path_z(), | ||||||||||||||||||||||||
| bun_sys::O::CREAT | bun_sys::O::WRONLY | bun_sys::O::TRUNC, | ||||||||||||||||||||||||
| 0o644, | ||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||
| out.write_all(&self.file_buf) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| pub(crate) fn add_inline_snapshot_to_write( | ||||||||||||||||||||||||
|
|
@@ -444,12 +460,9 @@ impl Snapshots { | |||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| let file = File { | ||||||||||||||||||||||||
| id: file_id, | ||||||||||||||||||||||||
| file: bun_sys::File::from_fd(fd), | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| let file = bun_sys::File::from_fd(fd); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let file_text: Vec<u8> = file.file.read_to_end().map_err(Error::from)?; | ||||||||||||||||||||||||
| let file_text: Vec<u8> = file.read_to_end().map_err(Error::from)?; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let source = | ||||||||||||||||||||||||
| bun_ast::Source::init_path_string(test_filename_z.as_bytes(), file_text.as_slice()); | ||||||||||||||||||||||||
|
|
@@ -798,7 +811,7 @@ impl Snapshots { | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // 4. write out result_text to the file | ||||||||||||||||||||||||
| if let Err(e) = file.file.seek_to(0) { | ||||||||||||||||||||||||
| if let Err(e) = file.seek_to(0) { | ||||||||||||||||||||||||
| log.add_error_fmt( | ||||||||||||||||||||||||
| &source, | ||||||||||||||||||||||||
| bun_ast::Loc { start: 0 }, | ||||||||||||||||||||||||
|
|
@@ -810,7 +823,7 @@ impl Snapshots { | |||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if let Err(e) = file.file.write_all(&result_text) { | ||||||||||||||||||||||||
| if let Err(e) = file.write_all(&result_text) { | ||||||||||||||||||||||||
| log.add_error_fmt( | ||||||||||||||||||||||||
| &source, | ||||||||||||||||||||||||
| bun_ast::Loc { start: 0 }, | ||||||||||||||||||||||||
|
|
@@ -822,7 +835,7 @@ impl Snapshots { | |||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if result_text.len() < file_text.len() { | ||||||||||||||||||||||||
| if bun_sys::ftruncate(file.file.handle, result_text.len() as i64).is_err() { | ||||||||||||||||||||||||
| if bun_sys::ftruncate(file.handle, result_text.len() as i64).is_err() { | ||||||||||||||||||||||||
| panic!("Failed to update inline snapshot: File was left in an invalid state"); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
@@ -831,89 +844,70 @@ impl Snapshots { | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| fn get_snapshot_file(&mut self, file_id: FileId) -> Result<bun_sys::Result<()>, Error> { | ||||||||||||||||||||||||
| if self._current_file.is_none() || self._current_file.as_ref().unwrap().id != file_id { | ||||||||||||||||||||||||
| self.write_snapshot_file()?; | ||||||||||||||||||||||||
| if self._current_file.as_ref().is_some_and(|file| file.id == file_id) { | ||||||||||||||||||||||||
| return Ok(bun_sys::Result::Ok(())); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| self.write_snapshot_file()?; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // avoid `Jest::runner()` (aliases `&mut TestRunner` over live `&mut self`). | ||||||||||||||||||||||||
| // SAFETY: see `parse_file` — raw-pointer projection to disjoint `.files` field. | ||||||||||||||||||||||||
| let test_file_source = unsafe { | ||||||||||||||||||||||||
| let p = Jest::RUNNER.read().expect("Jest runner not set").as_ptr(); | ||||||||||||||||||||||||
| &(*p).files.items_source()[file_id as usize] | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| let name = test_file_source.path.name(); | ||||||||||||||||||||||||
| let test_filename = name.filename; | ||||||||||||||||||||||||
| let dir_path = name.dir_with_trailing_slash(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let mut snapshot_file_path_buf = bun_paths::path_buffer_pool::get(); | ||||||||||||||||||||||||
| let buf = snapshot_file_path_buf.0.as_mut_slice(); | ||||||||||||||||||||||||
| let mut pos = 0usize; | ||||||||||||||||||||||||
| buf[pos..pos + dir_path.len()].copy_from_slice(dir_path); | ||||||||||||||||||||||||
| pos += dir_path.len(); | ||||||||||||||||||||||||
| buf[pos..pos + Self::SNAPSHOTS_DIR_NAME.len()] | ||||||||||||||||||||||||
| .copy_from_slice(Self::SNAPSHOTS_DIR_NAME); | ||||||||||||||||||||||||
| pos += Self::SNAPSHOTS_DIR_NAME.len(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let cached_dir = self.snapshot_dir_path; | ||||||||||||||||||||||||
| if cached_dir.is_none() || !strings::eql_long(dir_path, cached_dir.unwrap(), true) { | ||||||||||||||||||||||||
| buf[pos] = 0; | ||||||||||||||||||||||||
| // SAFETY: buf[pos] == 0 written above | ||||||||||||||||||||||||
| let snapshot_dir_path = ZStr::from_buf(&buf[..], pos); | ||||||||||||||||||||||||
| match bun_sys::mkdir(snapshot_dir_path, 0o777) { | ||||||||||||||||||||||||
| bun_sys::Result::Ok(()) => { | ||||||||||||||||||||||||
| self.snapshot_dir_path = Some(dir_path); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| bun_sys::Result::Err(err) => match err.get_errno() { | ||||||||||||||||||||||||
| bun_sys::Errno::EEXIST => { | ||||||||||||||||||||||||
| self.snapshot_dir_path = Some(dir_path); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| _ => return Ok(bun_sys::Result::Err(err)), | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| // do NOT call `Jest::runner()` here — it hands out an exclusive ref to the global TestRunner, | ||||||||||||||||||||||||
| // and `self: &mut Snapshots` is a live borrow of that same TestRunner's `.snapshots` | ||||||||||||||||||||||||
| // field. Retagging the whole TestRunner would invalidate `self` under Stacked Borrows. | ||||||||||||||||||||||||
| // Project the disjoint `.files` sibling through the raw `RUNNER` pointer instead. | ||||||||||||||||||||||||
| // SAFETY: single-threaded JS VM; RUNNER is set before any Snapshots method runs | ||||||||||||||||||||||||
| // (Snapshots is a field of TestRunner). Raw-pointer place projection touches only | ||||||||||||||||||||||||
| // `.files` bytes, disjoint from `&mut self`. | ||||||||||||||||||||||||
| let test_file_source = unsafe { | ||||||||||||||||||||||||
| let p = Jest::RUNNER.read().expect("Jest runner not set").as_ptr(); | ||||||||||||||||||||||||
| &(*p).files.items_source()[file_id as usize] | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| let name = test_file_source.path.name(); | ||||||||||||||||||||||||
| let test_filename = name.filename; | ||||||||||||||||||||||||
| let test_dir = name.dir_with_trailing_slash(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let mut path: Vec<u8> = Vec::with_capacity( | ||||||||||||||||||||||||
| test_dir.len() | ||||||||||||||||||||||||
| + Self::SNAPSHOTS_DIR_NAME.len() | ||||||||||||||||||||||||
| + test_filename.len() | ||||||||||||||||||||||||
| + b".snap".len() | ||||||||||||||||||||||||
| + 1, | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| path.extend_from_slice(test_dir); | ||||||||||||||||||||||||
| path.extend_from_slice(Self::SNAPSHOTS_DIR_NAME); | ||||||||||||||||||||||||
| path.extend_from_slice(test_filename); | ||||||||||||||||||||||||
| path.extend_from_slice(b".snap"); | ||||||||||||||||||||||||
| path.push(0); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let file = File { | ||||||||||||||||||||||||
| id: file_id, | ||||||||||||||||||||||||
| test_dir, | ||||||||||||||||||||||||
| path, | ||||||||||||||||||||||||
| dirty: false, | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| buf[pos..pos + test_filename.len()].copy_from_slice(test_filename); | ||||||||||||||||||||||||
| pos += test_filename.len(); | ||||||||||||||||||||||||
| buf[pos..pos + b".snap".len()].copy_from_slice(b".snap"); | ||||||||||||||||||||||||
| pos += b".snap".len(); | ||||||||||||||||||||||||
| buf[pos] = 0; | ||||||||||||||||||||||||
| // SAFETY: buf[pos] == 0 written above | ||||||||||||||||||||||||
| let snapshot_file_path = ZStr::from_buf(&buf[..], pos); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let mut flags: i32 = bun_sys::O::CREAT | bun_sys::O::RDWR; | ||||||||||||||||||||||||
| if self.update_snapshots { | ||||||||||||||||||||||||
| flags |= bun_sys::O::TRUNC; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| let fd = match bun_sys::open(snapshot_file_path, flags, 0o644) { | ||||||||||||||||||||||||
| bun_sys::Result::Ok(fd) => fd, | ||||||||||||||||||||||||
| // The file is read here and created in `write_snapshot_file`, once an | ||||||||||||||||||||||||
| // entry was added. A run that adds nothing (for example in CI, where | ||||||||||||||||||||||||
| // new snapshots are refused) leaves the disk as it found it. | ||||||||||||||||||||||||
| let existing: Vec<u8> = if self.update_snapshots { | ||||||||||||||||||||||||
| Vec::new() | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| match bun_sys::File::open(file.path_z(), bun_sys::O::RDONLY, 0) | ||||||||||||||||||||||||
| .and_then(|existing| existing.read_to_end()) | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| bun_sys::Result::Ok(contents) => contents, | ||||||||||||||||||||||||
| bun_sys::Result::Err(err) if err.get_errno() == bun_sys::Errno::ENOENT => Vec::new(), | ||||||||||||||||||||||||
| bun_sys::Result::Err(err) => return Ok(bun_sys::Result::Err(err)), | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let file = File { | ||||||||||||||||||||||||
| id: file_id, | ||||||||||||||||||||||||
| file: bun_sys::File::from_fd(fd), | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if self.update_snapshots { | ||||||||||||||||||||||||
| self.file_buf.extend_from_slice(Self::FILE_HEADER); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| let length = file.file.get_end_pos().map_err(Error::from)?; | ||||||||||||||||||||||||
| if length == 0 { | ||||||||||||||||||||||||
| self.file_buf.extend_from_slice(Self::FILE_HEADER); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| let mut tmp = vec![0u8; length]; | ||||||||||||||||||||||||
| let _ = file.file.pread_all(&mut tmp, 0).map_err(Error::from)?; | ||||||||||||||||||||||||
| #[cfg(windows)] | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| file.file.seek_to(0).map_err(Error::from)?; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| self.file_buf.extend_from_slice(&tmp); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.parse_file(&file)?; | ||||||||||||||||||||||||
| self._current_file = Some(file); | ||||||||||||||||||||||||
| if existing.is_empty() { | ||||||||||||||||||||||||
| self.file_buf.extend_from_slice(Self::FILE_HEADER); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| self.file_buf = existing; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+902
to
906
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Clear
Clear the buffer before you populate it. 🐛 Proposed fix if existing.is_empty() {
+ self.file_buf.clear();
self.file_buf.extend_from_slice(Self::FILE_HEADER);
} else {
self.file_buf = existing;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.parse_file(&file)?; | ||||||||||||||||||||||||
| self._current_file = Some(file); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Ok(bun_sys::Result::Ok(())) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 149
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 16198
🤖 get_repo_knowledge executed:
get_repo_knowledge oven-sh/bun /tmp/coderabbit-repo-knowledge/oven-sh-bun-efe5fbb6/learnings /tmp/coderabbit-repo-knowledge/oven-sh-bun-efe5fbb6/conventionsLength of output: 46751
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 21742
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 17977
🌐 Web query:
libuv uv_fs_mkdir Windows trailing slash path CreateDirectoryW💡 Result:
In libuv,
uv_fs_mkdiron Windows internally utilizes the Win32CreateDirectoryWAPI [1][2]. Regarding the behavior ofCreateDirectoryWwith trailing slashes, it is important to note the following: 1. Path Handling: The WindowsCreateDirectoryWfunction generally does not support a trailing backslash or forward slash for the path argument [3]. If a path ends in a trailing slash,CreateDirectoryWtypically fails withERROR_INVALID_NAMEor equivalent errors, as the system expects a valid directory name component and not a path terminator [3][4]. 2. Libuv's Implementation: Historically, libuv moved from using the C runtime function_wmkdirtoCreateDirectoryWto better handle wide-character paths and file modes [1][2]. Libuv does not explicitly strip trailing slashes in its generaluv_fs_mkdirimplementation; therefore, if the input path passed touv_fs_mkdircontains a trailing slash, it will be passed directly to the underlyingCreateDirectoryWcall, which will likely result in a failure [3]. 3. Context in Junctions/Symlinks: While recent libuv development has involved discussions and changes regarding trailing slashes (specifically in the context of junctions and symbolic links to ensure compatibility) [5][6], these changes are specific to those subsystems and do not imply thatuv_fs_mkdirarbitrarily handles trailing slashes for standard directory creation [5]. In summary, when usinguv_fs_mkdiron Windows, you should ensure the input path does not contain a trailing slash, as the underlyingCreateDirectoryWAPI will not process it correctly [3][4].Citations:
Remove the trailing separator before
bun_sys::mkdir. On Windows,SNAPSHOTS_DIR_NAMEends with\.snapshots_dir_zincludes that separator, andcreate_snapshot_filepasses the path tobun_sys::mkdir, which uses libuv andCreateDirectoryW.CreateDirectoryWrejects a path that ends with\, so snapshot directory creation can fail on Windows. Build themkdirpath without the final separator, while retaining it inFile.pathfor filename concatenation.🤖 Prompt for AI Agents