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

test: basic functionality of readUIntBE() #10417

Closed
wants to merge 1 commit into from
Closed

test: basic functionality of readUIntBE() #10417

wants to merge 1 commit into from

Conversation

larissayvette
Copy link
Contributor

Checklist
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Testing readUIntBE() when noAssert is true and false

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 22, 2016
@Fishrock123
Copy link
Contributor

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

@italoacasas italoacasas added the buffer Issues and PRs related to the buffer subsystem. label Dec 23, 2016
@italoacasas
Copy link
Contributor

@nodejs/buffer

Trott
Trott approved these changes Dec 23, 2016
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM.

Based on review of coverage.nodejs.org, this test increases the test coverage in lib/buffer.js. (@larissayvette: You might want to mention that in your PR descriptions when opening these. No big deal if you don't, but probably better if you do.)

julianduque
julianduque suggested changes Dec 23, 2016
View reviewed changes
test/parallel/test-buffer-readuintbe.js
assert.throws(
() => {
buf.readUIntBE(5);
}, RangeError
Copy link
Contributor

@julianduque julianduque Dec 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This should follow the same recommendations @jasnell did here: #10359 (review) to maintain consistency

Copy link
Member

Choose a reason for hiding this comment

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

@julianduque Can you be a bit more specific? To me, this seems OK if it's indented two spaces, which it seems to be here.

Are you saying the arrow function should be all on one line?

Or are you saying that the arrow function should start one line above, right after the parenthesis?

Or something else?

Copy link
Contributor

@julianduque julianduque Dec 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@Trott Sorry if I wasn't clear enough, I'm talking about matching the error message instead the error type.

Quoting @jasnell

Testing the error message would be more reliable here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see! Thanks.

@julianduque
Copy link
Contributor

CI 2: https://ci.nodejs.org/job/node-test-commit/6836/

@julianduque
Copy link
Contributor

Landed in a5103ce Thanks!

julianduque pushed a commit that referenced this pull request Dec 23, 2016
@larissayvette @julianduque
test: basic functionality of readUIntBE()
a5103ce 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
@larissayvette larissayvette deleted the test-readiuintbe branch December 28, 2016 15:52
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@larissayvette @evanlucas
test: basic functionality of readUIntBE()
edc0267 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@larissayvette @evanlucas
test: basic functionality of readUIntBE()
826decf 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@larissayvette @MylesBorins
test: basic functionality of readUIntBE()
dbfc103 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
@MylesBorins
Copy link
Contributor

this test is failing on v4.x

=== release test-buffer-readuintbe ===
Path: parallel/test-buffer-readuintbe
assert.js:354
    throw actual;
    ^

RangeError: index out of range
    at checkOffset (buffer.js:696:11)
    at Buffer.readUIntBE (buffer.js:720:5)
    at /Users/thealphanerd/code/node/v4.x/test/parallel/test-buffer-readuintbe.js:14:9
    at _tryBlock (assert.js:313:5)
    at _throws (assert.js:332:12)
    at Function.assert.throws (assert.js:362:3)
    at Object.<anonymous> (/Users/thealphanerd/code/node/v4.x/test/parallel/test-buffer-readuintbe.js:12:8)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
Command: out/Release/node /Users/thealphanerd/code/node/v4.x/test/parallel/test-buffer-readuintbe.js

@Trott
Copy link
Member

Trott commented Jan 23, 2017

It looks like the test is failing because the error on v4.x contains the string "index" while it is "Index" on current master. As it turns out, a later PR removes this test anyway, so I'll label this and the other one as "do not land on v4.x".

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@larissayvette @MylesBorins
test: basic functionality of readUIntBE()
3858b60 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@larissayvette @MylesBorins
test: basic functionality of readUIntBE()
9546ad7 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@italoacasas italoacasas italoacasas approved these changes

@julianduque julianduque julianduque approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@larissayvette @Fishrock123 @italoacasas @julianduque @MylesBorins @Trott @jasnell @nodejs-github-bot