test: fix broken tests in test-buffer-includes #12040

aqrln · 2017-03-25T18:03:30Z

Some of the tests for buffer.includes() functionality introduced in #3567 have been broken in a way that caused them to always pass regardless of the result of the tested method.

This behavior was caused by two reasons:

These tests were written as though buffer.includes() was supposed to return the same value that buffer.indexOf() does, i.e., used indices or -1 as expected return values instead of true and false.
assert() was used as the assertion function to do that instead of assert.strictEqual().

Thus assert() was called with a non-zero number as the first argument effectively causing these tests to pass.

This commit changes the tests to use assert.ok() and assert.ifError() and removes redundant indices.

Refs: #3567

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

Trott

LGTM if CI is ✅. /cc @nodejs/testing

lpinca · 2017-03-26T08:03:23Z

test/parallel/test-buffer-includes.js

+  mixedByteStringUcs2.includes(Buffer.from('bc', 'ucs2'), 0, 'ucs2'));
+assert.ok(
+  mixedByteStringUcs2.includes(Buffer.from('\u03a3', 'ucs2'), 0, 'ucs2'));
+assert.ifError(


Nit: I would use assert.ok(!...) for consistency and proper thrown error in case of failure

aqrln · 2017-03-26T15:33:34Z

Updated the PR to address @lpinca's feedback.

cjihrig

I can never remember if the indentation should be two or four spaces in these cases, but it looks like this PR uses both. Could you pick one or the other. Other than that, LGTM.

Some of the tests for `buffer.includes()` functionality introduced in nodejs#3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. Refs: nodejs#3567

aqrln · 2017-03-26T16:17:58Z

@cjihrig thanks for catching that, the PR has been updated. I've chosen four for this commit because it is more common throughout the file, but it is not consistent even in existing code. I guess I'll open a new PR that fixes the code style in this file.

hiroppy · 2017-03-26T17:25:35Z

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

aqrln · 2017-03-26T18:13:02Z

Failed tests on CentOS 5:

parallel/test-dns-regress-6244
parallel/test-net-connect-options-ipv6

and on Windows:

parallel/test-child-process-exec-kill-throws

are unrelated to this PR, but it think we should investigate those tests.

Troubles on test/arm-fanned (pi1-raspbian-wheezy):

java.io.IOException: Failed to create a temp file on /home/iojs/build/workspace/node-test-binary-arm
java.io.IOException: No space left on device

gibfahn · 2017-03-26T20:45:32Z

@cjihrig I think it's 4 spaces for run-on lines, so that if you have:

while ("Some really really really really really long string" === 
    somethingElse.toString())
  doThis();

The somethingElse.toString() is clearly separate from the doThis().

Seems like something that should be in the STYLE_GUIDE though (except that the STYLE_GUIDE currently doesn't cover code), I assume we don't lint for it because sometimes we want visual linting, but I'm not sure (cc/ @not-an-aardvark, is this something that we could easily lint for)?

Visual linting:

while ("Some really really really really really long string" === 
       somethingElse.toString())
  doThis();

gibfahn · 2017-03-26T20:45:52Z

@aqrln parallel/test-child-process-exec-kill-throws failure is unrelated to this PR, see #11038 (comment)

not-an-aardvark · 2017-03-26T20:50:26Z

(cc/ @not-an-aardvark, is this something that we could easily lint for)?

The built-in indent rule doesn't check for this, but we could probably make a custom rule for it if we wanted to.

aqrln · 2017-03-26T21:07:48Z

@gibfahn I think we can lint that run-on lines are indented with at least four spaces. That should cover both cases.

aqrln · 2017-03-27T00:48:00Z

dont-land-on-v4.x and lts-watch-v6.x labels should be added, I suppose.

aqrln · 2017-03-27T08:27:51Z

@gibfahn is the dont-land-on-v6.x label intentional? It cherry-picks cleanly to v6.x-staging but maybe I'm missing something.

targos · 2017-03-27T08:31:21Z

That was probably a mistake. I changed the label.

jasnell · 2017-03-27T17:27:58Z

CI failures are unrelated

Some of the tests for `buffer.includes()` functionality introduced in #3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. PR-URL: #12040 Ref: #3567 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell · 2017-03-27T17:32:16Z

Landed in 52b666e

aqrln · 2017-03-28T16:11:08Z

@jasnell btw, I see that you changed Refs: to Ref: in the commit message. I'm fine with both, but I thought Refs: was standardized in #10670.

gibfahn · 2017-03-28T16:26:23Z

@aqrln Ref is what node-review uses.

Ref: is still okay, we just suggest Refs: because it was used by more people.

EDIT: nodejs/node-review#9 has landed, so we should be good going forward!

aqrln · 2017-03-28T16:33:39Z

@gibfahn okay, thanks for clarifying this.

This was standardised in nodejs/node#10670. Refs: nodejs/node#12040

Some of the tests for `buffer.includes()` functionality introduced in #3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. PR-URL: #12040 Ref: #3567 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>

This was standardised in nodejs/node#10670. Refs: nodejs/node#12040

Some of the tests for `buffer.includes()` functionality introduced in #3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. PR-URL: #12040 Ref: #3567 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>

Some of the tests for `buffer.includes()` functionality introduced in nodejs/node#3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. PR-URL: nodejs/node#12040 Ref: nodejs/node#3567 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 25, 2017

Trott approved these changes Mar 25, 2017

View reviewed changes

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Mar 26, 2017

addaleax approved these changes Mar 26, 2017

View reviewed changes

lpinca approved these changes Mar 26, 2017

View reviewed changes

aqrln force-pushed the fix-test-buffer-includes branch from aaf7a0a to 9e040a1 Compare March 26, 2017 15:25

cjihrig approved these changes Mar 26, 2017

View reviewed changes

aqrln force-pushed the fix-test-buffer-includes branch from 9e040a1 to b9977cf Compare March 26, 2017 16:15

hiroppy approved these changes Mar 26, 2017

View reviewed changes

santigimeno approved these changes Mar 26, 2017

View reviewed changes

gibfahn added dont-land-on-v4.x labels Mar 27, 2017

targos added lts-watch-v6.x and removed dont-land-on-v6.x labels Mar 27, 2017

jasnell approved these changes Mar 27, 2017

View reviewed changes

jasnell closed this Mar 27, 2017

aqrln deleted the fix-test-buffer-includes branch March 27, 2017 17:33

gibfahn added a commit to gibfahn/node-review that referenced this pull request Mar 28, 2017

Use Refs instead of Ref

7edb7cc

This was standardised in nodejs/node#10670. Refs: nodejs/node#12040

gibfahn mentioned this pull request Mar 28, 2017

Use Refs instead of Ref nodejs/node-review#9

Merged

MylesBorins mentioned this pull request Mar 28, 2017

V7.8.0 proposal #12104

Merged

evanlucas pushed a commit to nodejs/node-review that referenced this pull request Mar 29, 2017

Use Refs instead of Ref (#9)

9b5b27b

This was standardised in nodejs/node#10670. Refs: nodejs/node#12040

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Apr 18, 2017

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix broken tests in test-buffer-includes #12040

test: fix broken tests in test-buffer-includes #12040

aqrln commented Mar 25, 2017

Trott left a comment

lpinca Mar 26, 2017

aqrln commented Mar 26, 2017

cjihrig left a comment

aqrln commented Mar 26, 2017

hiroppy commented Mar 26, 2017

aqrln commented Mar 26, 2017 •

edited

Loading

gibfahn commented Mar 26, 2017

gibfahn commented Mar 26, 2017 •

edited

Loading

not-an-aardvark commented Mar 26, 2017

aqrln commented Mar 26, 2017

aqrln commented Mar 27, 2017

aqrln commented Mar 27, 2017

targos commented Mar 27, 2017

jasnell commented Mar 27, 2017

jasnell commented Mar 27, 2017

aqrln commented Mar 28, 2017

gibfahn commented Mar 28, 2017 •

edited

Loading

aqrln commented Mar 28, 2017

test: fix broken tests in test-buffer-includes #12040

test: fix broken tests in test-buffer-includes #12040

Conversation

aqrln commented Mar 25, 2017

Checklist

Affected core subsystem(s)

Trott left a comment

Choose a reason for hiding this comment

lpinca Mar 26, 2017

Choose a reason for hiding this comment

aqrln commented Mar 26, 2017

cjihrig left a comment

Choose a reason for hiding this comment

aqrln commented Mar 26, 2017

hiroppy commented Mar 26, 2017

aqrln commented Mar 26, 2017 • edited Loading

gibfahn commented Mar 26, 2017

gibfahn commented Mar 26, 2017 • edited Loading

not-an-aardvark commented Mar 26, 2017

aqrln commented Mar 26, 2017

aqrln commented Mar 27, 2017

aqrln commented Mar 27, 2017

targos commented Mar 27, 2017

jasnell commented Mar 27, 2017

jasnell commented Mar 27, 2017

aqrln commented Mar 28, 2017

gibfahn commented Mar 28, 2017 • edited Loading

aqrln commented Mar 28, 2017

aqrln commented Mar 26, 2017 •

edited

Loading

gibfahn commented Mar 26, 2017 •

edited

Loading

gibfahn commented Mar 28, 2017 •

edited

Loading