test: basic functionality of readUIntLE() #10359

larissayvette · 2016-12-20T19:48:31Z

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

julianduque · 2016-12-20T21:17:45Z

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

Trott

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

cjihrig · 2016-12-21T00:48:37Z

test/parallel/test-buffer-readuintle.js

+assert.strictEqual(result, 168);
+
+assert.throws(
+      () => { buf.readUIntLE(5);}, RangeError);


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

cjihrig · 2016-12-21T00:49:03Z

test/parallel/test-buffer-readuintle.js

+      () => { buf.readUIntLE(5);}, RangeError);
+
+assert.doesNotThrow(
+      () => { assert.strictEqual(buf.readUIntLE(5, 0, true), undefined);},


Same comment here.

julianduque · 2016-12-22T02:22:44Z

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

Trott · 2016-12-22T05:07:27Z

test/parallel/test-buffer-readuintle.js

+assert.strictEqual(result, 168);
+
+assert.throws(
+      () => { buf.readUIntLE(5); }, RangeError);


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.

Trott · 2016-12-22T05:09:39Z

test/parallel/test-buffer-readuintle.js

+      () => { buf.readUIntLE(5); }, RangeError);
+
+assert.doesNotThrow(
+      () => { assert.strictEqual(buf.readUIntLE(5, 0, true), undefined); },


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.

Trott · 2016-12-22T05:10:11Z

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

larissayvette · 2016-12-22T21:45:05Z

@Trott I modified the linting as you said

Trott · 2016-12-22T21:56:16Z

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

jasnell · 2016-12-23T07:32:58Z

test/parallel/test-buffer-readuintle.js

+assert.strictEqual(result, 168);
+
+assert.throws(
+      () => {


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?

jasnell · 2016-12-23T07:33:20Z

test/parallel/test-buffer-readuintle.js

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


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$/ )

larissayvette · 2016-12-23T17:02:07Z

@jasnell I made the changes

jasnell · 2016-12-23T20:30:20Z

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

jasnell · 2016-12-24T07:31:04Z

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

jasnell · 2016-12-24T21:11:49Z

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

Trott · 2016-12-24T23:19:32Z

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

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

Trott · 2016-12-24T23:28:52Z

(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>

Trott · 2016-12-24T23:45:29Z

Landed in 66c0767

Trott · 2016-12-24T23:45:56Z

🎉

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>

MylesBorins · 2017-01-23T02:40:00Z

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

Trott · 2017-01-23T04:16:51Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: basic functionality of readUIntLE() #10359

test: basic functionality of readUIntLE() #10359

larissayvette commented Dec 20, 2016 •

edited

Loading

julianduque commented Dec 20, 2016

Trott left a comment

cjihrig Dec 21, 2016

Trott Dec 21, 2016

larissayvette Dec 21, 2016

cjihrig Dec 21, 2016

julianduque commented Dec 22, 2016

Trott Dec 22, 2016

Trott Dec 22, 2016

Trott Dec 22, 2016

Trott commented Dec 22, 2016

larissayvette commented Dec 22, 2016

Trott commented Dec 22, 2016

jasnell Dec 23, 2016

Trott Dec 23, 2016

jasnell Dec 23, 2016

larissayvette Dec 23, 2016

julianduque Dec 23, 2016

larissayvette Dec 23, 2016

Trott Dec 23, 2016 •

edited

Loading

larissayvette commented Dec 23, 2016

jasnell commented Dec 23, 2016 via email

jasnell commented Dec 24, 2016

jasnell commented Dec 24, 2016

Trott commented Dec 24, 2016

Trott commented Dec 24, 2016

Trott commented Dec 24, 2016

Trott commented Dec 24, 2016

MylesBorins commented Jan 23, 2017

Trott commented Jan 23, 2017 •

edited

Loading

test: basic functionality of readUIntLE() #10359

test: basic functionality of readUIntLE() #10359

Conversation

larissayvette commented Dec 20, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

julianduque commented Dec 20, 2016

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianduque commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Dec 22, 2016

larissayvette commented Dec 22, 2016

Trott commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Dec 23, 2016 • edited Loading

Choose a reason for hiding this comment

larissayvette commented Dec 23, 2016

jasnell commented Dec 23, 2016 via email

jasnell commented Dec 24, 2016

jasnell commented Dec 24, 2016

Trott commented Dec 24, 2016

Trott commented Dec 24, 2016

Trott commented Dec 24, 2016

Trott commented Dec 24, 2016

MylesBorins commented Jan 23, 2017

Trott commented Jan 23, 2017 • edited Loading

larissayvette commented Dec 20, 2016 •

edited

Loading

Trott Dec 23, 2016 •

edited

Loading

Trott commented Jan 23, 2017 •

edited

Loading