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.

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

test: basic functionality of readUIntBE() #10417

Closed
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

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

@nodejs/buffer

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.)

assert.throws(
() => {
buf.readUIntBE(5);
}, RangeError
Copy link
Contributor

@julianduque julianduque Dec 23, 2016

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

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

@julianduque
Copy link
Contributor

Landed in a5103ce Thanks!

julianduque pushed a commit that referenced this pull request Dec 23, 2016
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 pushed a commit that referenced this pull request Jan 3, 2017
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
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
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
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
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
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
Labels
buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants