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

Closed
wants to merge 2 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott 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

* favor `assert.strictEqual()` and friends of `assert.equal()` etc.
* favor `.includes()` over `.indexOf()` for existence checks
@Trott Trott added util Issues and PRs related to the built-in util module. test Issues and PRs related to the tests. labels Aug 19, 2016
@fhinkel
Copy link
Member

fhinkel commented Aug 21, 2016

@fhinkel
Copy link
Member

fhinkel commented Aug 21, 2016

LGTM

assert.ok(ex.indexOf('[message]') != -1);
assert.strictEqual(ex.includes('Error: FAIL'), true);
assert.strictEqual(ex.includes('[stack]'), true);
assert.strictEqual(ex.includes('[message]'), true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to change these but I think it's a good case for assert.ok()

@targos
Copy link
Member

targos commented Aug 21, 2016

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Aug 21, 2016

LGTM

@addaleax
Copy link
Member

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

@jbergstroem
Copy link
Member

jbergstroem commented Aug 22, 2016

@addaleax probably need to revisit logic regarding flaky.

@Trott
Copy link
Member Author

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
Copy link
Member Author

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
* 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
Copy link
Member Author

Trott commented Aug 23, 2016

Landed in e3cb0bf

@Trott Trott closed this Aug 23, 2016
@Fishrock123 Fishrock123 mentioned this pull request Sep 6, 2016
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 7, 2016
* 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 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
* 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 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
* 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>
@Trott Trott deleted the assert-refactor branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants