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.

Sign up for GitHub

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

Jump to bottom

replace ==, !=, some instances of var, and assert.equal() #8770

Closed
wants to merge 1 commit into from
Closed

replace ==, !=, some instances of var, and assert.equal() #8770

wants to merge 1 commit into from

Conversation

jennabelle
Copy link
Contributor

@jennabelle jennabelle commented Sep 25, 2016 •
edited by imyller
Loading

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change
  • replaced == with ===
  • replaced != with !==
  • replaced some var instances with const
  • replaced assert.equal() with assert.strictEqual()

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 25, 2016
This was referenced Sep 25, 2016
Closed
Closed
not-an-aardvark
not-an-aardvark approved these changes Sep 25, 2016
View reviewed changes
Copy link
Contributor

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

LGTM

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

@Trott
Copy link
Member

Trott commented Sep 25, 2016

LGTM if CI is green

lpinca
lpinca approved these changes Sep 25, 2016
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Sep 25, 2016
@jennabelle
Copy link
Contributor Author

It looks like there is an error in Jenkins (from here):

# FATAL ERROR: invalid array length Allocation failed - JavaScript heap out of memory

@addaleax
Copy link
Member

@jennabelle That’s not related to this PR, we’ve been having problems with that particular test for a while. So, no need to worry!

imyller
imyller approved these changes Sep 25, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@imyller imyller self-assigned this Sep 27, 2016
@imyller
Copy link
Member

imyller commented Sep 27, 2016

I'll start landing this:

  • Five LGTMs
  • No objections
  • CI tests passed (only known/unrelated CI failures)

imyller pushed a commit to imyller/node that referenced this pull request Sep 27, 2016
@imyller
test: cleanup parallel/test-file-write-stream2.js
782db08 
replace ==, !=, some instances of var, and assert.equal()

PR-URL: nodejs#8770
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller
Copy link
Member

imyller commented Sep 27, 2016

landed in 782db08

Thank you for your effort and contribution, @jennabelle

@imyller imyller closed this Sep 27, 2016
@imyller imyller removed their assignment Sep 27, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
@jasnell
test: cleanup parallel/test-file-write-stream2.js
7f46867 
replace ==, !=, some instances of var, and assert.equal()

PR-URL: #8770
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@Fishrock123
test: cleanup parallel/test-file-write-stream2.js
f523b82 
replace ==, !=, some instances of var, and assert.equal()

PR-URL: #8770
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@imyller imyller imyller approved these changes

@cjihrig cjihrig cjihrig approved these changes

@not-an-aardvark not-an-aardvark not-an-aardvark approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@jennabelle @Trott @addaleax @imyller @lpinca @cjihrig @not-an-aardvark @mscdex @MylesBorins @nodejs-github-bot