Skip to content
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.

Sign up for GitHub

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

Jump to bottom

buffer: throw when writing beyond buffer #54588

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

buffer: throw when writing beyond buffer #54588

wants to merge 1 commit into from
+19 −6

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 27, 2024

This reverts commit dd8eeec.

@ronag ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 27, 2024
@ronag ronag requested a review from mcollina August 27, 2024 08:59
@ronag ronag added the buffer Issues and PRs related to the buffer subsystem. label Aug 27, 2024
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 27, 2024
mcollina
mcollina approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag requested a review from targos August 27, 2024 09:35
@ronag ronag changed the title Revert "buffer: truncate instead of throw when writing beyond buffer" buffer: Throw when writing beyond buffer" Aug 27, 2024
ronag added a commit to nxtedition/node that referenced this pull request Aug 27, 2024
@ronag
buffer: Throw when writing beyond buffer"
6535f47 
This reverts commit dd8eeec.

PR-URL: nodejs#54588
Refs: nodejs#54524
ronag added a commit to nxtedition/node that referenced this pull request Aug 27, 2024
@ronag
buffer: Throw when writing beyond buffer"
2a80f7d 
This reverts commit dd8eeec.

PR-URL: nodejs#54588
Refs: nodejs#54524
@codecov Codecov
Copy link

codecov bot commented Aug 27, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.33%. Comparing base (dd8eeec) to head (2edea24).
Report is 147 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/buffer.js 66.66% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #54588   +/-   ##
=======================================
  Coverage   87.32%   87.33%           
=======================================
  Files         649      649           
  Lines      182621   182632   +11     
  Branches    35042    35040    -2     
=======================================
+ Hits       159480   159498   +18     
- Misses      16397    16398    +1     
+ Partials     6744     6736    -8
Files with missing lines Coverage Δ
lib/internal/buffer.js 97.41% <66.66%> (ø)

... and 30 files with indirect coverage changes

@ronag ronag changed the title buffer: Throw when writing beyond buffer" buffer: Throw when writing beyond buffer Aug 27, 2024
@ronag ronag changed the title buffer: Throw when writing beyond buffer buffer: throw when writing beyond buffer Aug 27, 2024
jasnell
jasnell reviewed Aug 27, 2024
View reviewed changes
test/parallel/test-buffer-write.js
@@ -121,3 +121,16 @@ assert.throws(() => {
}, common.expectsError({
code: 'ERR_BUFFER_OUT_OF_BOUNDS',
}));


assert.throws(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: as someone who is actively needing to go through and port these tests to other environments in an attempt to be more compatible with node.js, having comments included with these additional tests would be helpful to identify what exactly is being tested.

@RedYetiDev RedYetiDev added the revert PRs that revert previously landed PRs. label Aug 28, 2024
@ShenHongFei ShenHongFei mentioned this pull request Aug 30, 2024
Merged
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61732/

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 31, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61737/

mcollina
mcollina approved these changes Aug 31, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@jasnell
Copy link
Member

jasnell commented Sep 8, 2024

PR is currently blocked from landing due to unreliable CI. Likely needs a rebase

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62206/

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

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. revert PRs that revert previously landed PRs. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ronag @nodejs-github-bot @jasnell @mcollina @targos @RedYetiDev