test: basic functionality of readUIntLE() by larissayvette · Pull Request #10359 · nodejs/node · GitHub

Checklist

commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

test readUIntLE() when noAsset is tfalse and when it is false

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

LGTM. This provides coverage for a code branch that is not currently covered.

Space between the ; and } please.

This the type of thing that could and should be enforced via linting. So let's do that: #10377

making the changes right away

Same comment here.

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

Any reason not to just make it all one line, like this?:

assert.throws(() => { buf.readIntLE(5); }, RangeError);

Or, if you prefer to split it up for readability, please indent by only two lines and put the closing parenthesis on a separate line:

assert.throws(
  () => { buf.readIntLE(5); }, RangeError
);

The indent-by-two-chars is something that should be caught by the linter but currently is not. I think it will start to be reported when we update our ESLint version. But there are a few obstacles to that at the moment.

As mentioned above, if you could make this indented by just two characters, that would likely be preferable. Again, sorry our linter doesn't yet report that.

@cjihrig This PR has been updated to address your comments. Can you take a look and, if appropriate, update your review?

@Trott I modified the linting as you said

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

minor nit: the formatting on this does not match the regular style...

@jasnell Can you be more specific? To my mind, this would be OK if the indentation on line 13 was two spaces rather than six spaces. Is that your opinion too?

Testing the error message would be more reliable here.

@jasnell I do not get what you mean

@larissayvette for example, doing a regex agains the error message thrown by RangeError in this case:

assert.throws(
  () => {
    buf.readUIntLE(5);
  }, /Index out of range/
)

ok thanks @jasnell

I would take it a step further and match the entire message from beginning to end, since (currently) any change to an error message is considered a breaking change.

assert.throws(
  () => {
    buf.readUIntLE(5);
  }, /^RangeError: Index out of range$/
)

@jasnell I made the changes

Yes, that is what I meant with regards to indentation

…

On Fri, Dec 23, 2016 at 9:09 AM Rich Trott ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/parallel/test-buffer-readuintle.js <#10359>: > @@ -0,0 +1,22 @@ +'use strict'; +require('../common'); +const assert = require('assert'); + +// testing basic functionality of readUIntLE() + +const buf = Buffer.from([42, 84, 168, 127]); +const result = buf.readUIntLE(2); + +assert.strictEqual(result, 168); + +assert.throws( + () => { @jasnell <https://github.com/jasnell> Can you be more specific? To my mind, this would be OK if the indentation on line 13 was two spaces rather than six spaces. Is that your opinion too? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10359>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2eei4QelzhBaf0UHiAWEKXwGDZLbIks5rLAA-gaJpZM4LSOf_> .

@cjihrig ... ping .. does this LGTY now?

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

Previous CI run seems to have terminated oddly. Let's try again...

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

(Failure on arm-fanned is CI infrastructure related and not a result of these changes.)

PR-URL: nodejs#10359 Reviewed-By: Julian Duque <julianduquej@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Landed in 66c0767

🎉

PR-URL: #10359 Reviewed-By: Julian Duque <julianduquej@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

this is failing on v4.x, is that expected?

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

RangeError: index out of range
    at checkOffset (buffer.js:696:11)
    at Buffer.readUIntLE (buffer.js:704:5)
    at /Users/thealphanerd/code/node/v4.x/test/parallel/test-buffer-readuintle.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-readuintle.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-readuintle.js

The error message changed between 4.x and 6.x. This test gets deleted by a subsequent PR anyway (already marked as do-not-land-on-v4.x) so I'll label this similarly.

PR-URL: #10359 Reviewed-By: Julian Duque <julianduquej@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 20, 2016

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Dec 20, 2016

julianduque approved these changes Dec 20, 2016

View reviewed changes

Trott approved these changes Dec 21, 2016

View reviewed changes

cjihrig requested changes Dec 21, 2016

View reviewed changes

larissayvette force-pushed the test-buffer branch from 0096611 to 4473247 Compare December 21, 2016 22:25

nodejs-github-bot added lts-watch-v6.x labels Dec 21, 2016

italoacasas approved these changes Dec 22, 2016

View reviewed changes

Trott reviewed Dec 22, 2016

View reviewed changes

larissayvette force-pushed the test-buffer branch from 4473247 to 284e552 Compare December 22, 2016 21:43

nodejs-github-bot added lts-watch-v6.x labels Dec 22, 2016

jasnell requested changes Dec 23, 2016

View reviewed changes

julianduque mentioned this pull request Dec 23, 2016

test: basic functionality of readUIntBE() #10417

Closed

1 task

larissayvette force-pushed the test-buffer branch from 284e552 to e025941 Compare December 23, 2016 17:00

nodejs-github-bot added lts-watch-v6.x labels Dec 23, 2016

test: basic functionality of readUIntLE()

258cc0e

larissayvette force-pushed the test-buffer branch from e025941 to 258cc0e Compare December 23, 2016 18:07

nodejs-github-bot added the lts-watch-v6.x label Dec 23, 2016

nodejs-github-bot added the lts-watch-v4.x label Dec 23, 2016

jasnell approved these changes Dec 24, 2016

View reviewed changes

cjihrig approved these changes Dec 24, 2016

View reviewed changes

Trott closed this Dec 24, 2016

larissayvette deleted the test-buffer branch December 28, 2016 15:51

evanlucas mentioned this pull request Jan 3, 2017

v7.4.0 release proposal #10589

Merged

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Jan 23, 2017

Trott added dont-land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v6.10.0 proposal #10974

Merged

test: basic functionality of readUIntLE() by larissayvette · Pull Request #10359 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: basic functionality of readUIntLE() - #10359

test: basic functionality of readUIntLE()#10359larissayvette wants to merge 1 commit intonodejs:masternodejs/node:masterfrom larissayvette:test-bufferlarissayvette/node:test-bufferCopy head branch name to clipboard

Conversation

larissayvette commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

julianduque commented Dec 20, 2016

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianduque commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 22, 2016

Uh oh!

larissayvette commented Dec 22, 2016

Uh oh!

Trott commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larissayvette commented Dec 23, 2016

Uh oh!

jasnell commented Dec 23, 2016 via email

Uh oh!

jasnell commented Dec 24, 2016

Uh oh!

jasnell commented Dec 24, 2016

Uh oh!

Trott commented Dec 24, 2016

Uh oh!

Trott commented Dec 24, 2016

Uh oh!

Trott commented Dec 24, 2016

Uh oh!

Trott commented Dec 24, 2016

Uh oh!

MylesBorins commented Jan 23, 2017

Uh oh!

Trott commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

test: basic functionality of readUIntLE()#10359
larissayvette wants to merge 1 commit into
nodejs:masterfrom
larissayvette:test-buffer

larissayvette commented Dec 20, 2016 •
edited
Loading

Trott Dec 23, 2016 •
edited
Loading

Trott commented Jan 23, 2017 •
edited
Loading