Skip to content

test: test undefined for util #22741

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from
Closed

test: test undefined for util #22741

wants to merge 1 commit into from

Conversation

ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Sep 7, 2018

Add some undefined tests for util.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 7, 2018
@mscdex
Copy link
Contributor

mscdex commented Sep 7, 2018

I believe these cases are already accounted for when no arguments are passed.

@devsnek
Copy link
Member

devsnek commented Sep 7, 2018

@mscdex given that some of these methods are backed by natives it seems reasonable to test no-args undefined vs explicit undefined.

@addaleax addaleax added the needs-ci PRs that need a full CI run. label Sep 14, 2018
@addaleax
Copy link
Member

@addaleax addaleax removed the needs-ci PRs that need a full CI run. label Sep 14, 2018
@joaocgreis
Copy link
Member

Last CI run was not successful for unrelated reasons (nodejs/build#1495).
Full rebuild: https://ci.nodejs.org/job/node-test-pull-request/17198/

@ZYSzys
Copy link
Member Author

ZYSzys commented Sep 15, 2018

Logically speaking, it shouldn’t be failed. What’s the real reason making CI failed ?

@joaocgreis
Copy link
Member

@ZYSzys the rebuild I started is good (only one flaky test making it yellow, but yellow means it can be ignored).

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

@mhdawson
Copy link
Member

CI is green

Landed in a00eee3

@mhdawson mhdawson closed this Sep 27, 2018
mhdawson pushed a commit that referenced this pull request Sep 27, 2018
PR-URL: #22741
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@ZYSzys ZYSzys deleted the test branch September 27, 2018 16:40
targos pushed a commit that referenced this pull request Sep 27, 2018
PR-URL: #22741
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
PR-URL: #22741
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.