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.js #11779

Closed
wants to merge 1 commit into from

Conversation

aqrln
Copy link
Contributor

@aqrln aqrln commented Mar 10, 2017

  • Enclose tests that used to introduce module-level variables into their own scopes.
  • Replace ES5 anonymous functions with arrow functions where it makes sense.
  • And make one arrow function a regular function thus fixing a bug in a getter inside an object created in "Array with dynamic properties" test. This getter has never been invoked though, so the test hasn't been failing.
  • Convert snake_case identifiers to camelCase.
  • Make some variable names more readable.
  • Replace regular expressions in maxArrayLength tests with simple assert.strictEquals() and assert('...'.endsWith()) checks, as suggested in util: change sparse arrays inspection format #11576 (comment).
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 10, 2017
* Enclose tests that used to introduce module-level variables into
  their own scopes.
* Replace ES5 anonymous functions with arrow functions where it makes
  sense.
* And make one arrow function a regular function thus fixing a bug in a
  getter inside an object created in "Array with dynamic properties"
  test.  This getter has never been invoked though, so the test hasn't been
  failing.
* Convert snake_case identifiers to camelCase.
* Make some variable names more readable.
* Replace regular expressions in maxArrayLength tests with simple
  assert.strictEquals() and assert(...endsWith()) checks, as suggested
  in <nodejs#11576 (comment)>.
@aqrln aqrln force-pushed the refactor-test-util-inspect branch from a8073c3 to fc23c8d Compare March 10, 2017 02:55
@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Mar 10, 2017
@jasnell
Copy link
Member

jasnell commented Mar 15, 2017

jasnell pushed a commit that referenced this pull request Mar 15, 2017
* Enclose tests that used to introduce module-level variables into
  their own scopes.
* Replace ES5 anonymous functions with arrow functions where it makes
  sense.
* And make one arrow function a regular function thus fixing a bug in a
  getter inside an object created in "Array with dynamic properties"
  test.  This getter has never been invoked though, so the test hasn't been
  failing.
* Convert snake_case identifiers to camelCase.
* Make some variable names more readable.
* Replace regular expressions in maxArrayLength tests with simple
  assert.strictEquals() and assert(...endsWith()) checks, as suggested
  in <#11576 (comment)>.

PR-URL: #11779
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 15, 2017

Landed in 3d7245c

@jasnell jasnell closed this Mar 15, 2017
@aqrln aqrln deleted the refactor-test-util-inspect branch March 15, 2017 04:54
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
* Enclose tests that used to introduce module-level variables into
  their own scopes.
* Replace ES5 anonymous functions with arrow functions where it makes
  sense.
* And make one arrow function a regular function thus fixing a bug in a
  getter inside an object created in "Array with dynamic properties"
  test.  This getter has never been invoked though, so the test hasn't been
  failing.
* Convert snake_case identifiers to camelCase.
* Make some variable names more readable.
* Replace regular expressions in maxArrayLength tests with simple
  assert.strictEquals() and assert(...endsWith()) checks, as suggested
  in <nodejs#11576 (comment)>.

PR-URL: nodejs#11779
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas
Copy link
Contributor

This PR need backport to v7

aqrln added a commit to aqrln/node that referenced this pull request Mar 21, 2017
* Enclose tests that used to introduce module-level variables into
  their own scopes.
* Replace ES5 anonymous functions with arrow functions where it makes
  sense.
* And make one arrow function a regular function thus fixing a bug in a
  getter inside an object created in "Array with dynamic properties"
  test.  This getter has never been invoked though, so the test hasn't been
  failing.
* Convert snake_case identifiers to camelCase.
* Make some variable names more readable.
* Replace regular expressions in maxArrayLength tests with simple
  assert.strictEquals() and assert(...endsWith()) checks, as suggested
  in <nodejs#11576 (comment)>.

PR-URL: nodejs#11779
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@aqrln
Copy link
Contributor Author

aqrln commented Mar 21, 2017

Backported to v7.x, but #11775 must land first prior to backporting this PR to v6.x.

@jasnell jasnell mentioned this pull request Apr 4, 2017
evanlucas pushed a commit that referenced this pull request May 2, 2017
* Enclose tests that used to introduce module-level variables into
  their own scopes.
* Replace ES5 anonymous functions with arrow functions where it makes
  sense.
* And make one arrow function a regular function thus fixing a bug in a
  getter inside an object created in "Array with dynamic properties"
  test.  This getter has never been invoked though, so the test hasn't been
  failing.
* Convert snake_case identifiers to camelCase.
* Make some variable names more readable.
* Replace regular expressions in maxArrayLength tests with simple
  assert.strictEquals() and assert(...endsWith()) checks, as suggested
  in <#11576 (comment)>.

PR-URL: #11779
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@evanlucas evanlucas mentioned this pull request May 2, 2017
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