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: refactor test-util-inspect #8189

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Trott
Member

Trott commented Aug 19, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test util

Description of change
  • favor assert.strictEqual() and friends of assert.equal() etc.
  • favor .includes() over .indexOf() for existence checks
test: refactor test-util-inspect
* favor `assert.strictEqual()` and friends of `assert.equal()` etc.
* favor `.includes()` over `.indexOf()` for existence checks

@Trott Trott added util test labels Aug 19, 2016

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Aug 21, 2016

Member

LGTM

Member

fhinkel commented Aug 21, 2016

LGTM

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Aug 21, 2016

Member

LGTM

Member

targos commented Aug 21, 2016

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 21, 2016

Member

LGTM

Member

jasnell commented Aug 21, 2016

LGTM

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 22, 2016

Member

@jbergstroem fyi, the CI run was green but the build status here is shown as pending

Member

addaleax commented Aug 22, 2016

@jbergstroem fyi, the CI run was green but the build status here is shown as pending

@jbergstroem

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

jbergstroem Aug 22, 2016

Member

@addaleax probably need to revisit logic regarding flaky.

Member

jbergstroem commented Aug 22, 2016

@addaleax probably need to revisit logic regarding flaky.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 22, 2016

Member

Did the assert.strictEqual() -> assert() per suggestion from @targos. New CI: https://ci.nodejs.org/job/node-test-pull-request/3794/

Member

Trott commented Aug 22, 2016

Did the assert.strictEqual() -> assert() per suggestion from @targos. New CI: https://ci.nodejs.org/job/node-test-pull-request/3794/

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 23, 2016

Member

CI issues are a soon-to-be-reverted known-flaky and one build failure.

Member

Trott commented Aug 23, 2016

CI issues are a soon-to-be-reverted known-flaky and one build failure.

Trott added a commit to Trott/io.js that referenced this pull request Aug 23, 2016

test: refactor test-util-inspect
* favor `assert.strictEqual()` and friends of `assert.equal()` etc.
* favor `.includes()` over `.indexOf()` for existence checks

PR-URL: nodejs#8189
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 23, 2016

Member

Landed in e3cb0bf

Member

Trott commented Aug 23, 2016

Landed in e3cb0bf

@Trott Trott closed this Aug 23, 2016

@addaleax addaleax referenced this pull request Aug 24, 2016

Closed

util: allow symbol-based custom inspection methods #8174

4 of 4 tasks complete

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

addaleax added a commit to addaleax/node that referenced this pull request Sep 7, 2016

test: refactor test-util-inspect
* favor `assert.strictEqual()` and friends of `assert.equal()` etc.
* favor `.includes()` over `.indexOf()` for existence checks

PR-URL: nodejs#8189
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

test: refactor test-util-inspect
* favor `assert.strictEqual()` and friends of `assert.equal()` etc.
* favor `.includes()` over `.indexOf()` for existence checks

PR-URL: nodejs#8189
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#8437
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

test: refactor test-util-inspect
* favor `assert.strictEqual()` and friends of `assert.equal()` etc.
* favor `.includes()` over `.indexOf()` for existence checks

PR-URL: nodejs#8189
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs#8437
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment