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

util: add regression test #19188

Closed
wants to merge 1 commit into from
Closed

util: add regression test #19188

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Mar 6, 2018 •
edited
Loading

This fixes util.isDate and util.isRegExp to abort in case more than
one argument is passed to the utility function.

This adds regression tests to make sure passing multiple args to these functions does not abort.

This got fixed in #18415.

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

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Mar 6, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 6, 2018

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

@BridgeAR BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 6, 2018
@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. blocked PRs that are blocked by other issues or PRs. and removed blocked PRs that are blocked by other issues or PRs. labels Mar 7, 2018
@BridgeAR BridgeAR changed the title util: fix util abort util: add regression test Mar 7, 2018
@BridgeAR
util: add regression tests
ba94410 
This makes sure util.isDate and util.isRegExp will not abort in case
more than one argument is passed to the utility function.
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 7, 2018

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

I changed the description because the original issue was already fixed in #18415.

@lpinca
Copy link
Member

lpinca commented Mar 7, 2018

Should subsystem in commit title be test?

@BridgeAR
Copy link
Member Author

Landed in 8181c60

@lpinca thanks a lot for the heads up. I changed that while landing.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 11, 2018
@BridgeAR
test: add regression tests
8181c60 
This makes sure util.isDate and util.isRegExp will not abort in case
more than one argument is passed to the utility function.

PR-URL: nodejs#19188
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR BridgeAR closed this Mar 11, 2018
@MylesBorins
Copy link
Contributor

This test is failing on v9.x-staging

=== release test-util ===
Path: parallel/test-util
out/Release/node[91401]: ../src/node_util.cc:44:void node::util::IsRegExp(const FunctionCallbackInfo<v8::Value> &): Assertion `(1) == (args.Length())' failed.
 1: node::Abort() [/Users/mborins/code/node/v9.x/out/Release/node]
 2: node::Assert(char const* const (*) [4]) [/Users/mborins/code/node/v9.x/out/Release/node]
 3: node::util::IsSet(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/mborins/code/node/v9.x/out/Release/node]
 4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/mborins/code/node/v9.x/out/Release/node]
 5: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/mborins/code/node/v9.x/out/Release/node]
 6: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/mborins/code/node/v9.x/out/Release/node]
 7: 0x2624623042fd
Command: out/Release/node --expose-internals /Users/mborins/code/node/v9.x/test/parallel/test-util.js

Should it be backported?

@BridgeAR
Copy link
Member Author

@MylesBorins see #19223 for 9.x and #19224 for 8.x.

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@BridgeAR @MayaLekova
test: add regression tests
4a8954a 
This makes sure util.isDate and util.isRegExp will not abort in case
more than one argument is passed to the utility function.

PR-URL: nodejs#19188
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR BridgeAR deleted the fix-util-abort branch April 1, 2019 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@evanlucas evanlucas evanlucas approved these changes

@daynin daynin daynin approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@BridgeAR @lpinca @MylesBorins @jasnell @evanlucas @daynin @addaleax @cjihrig @nodejs-github-bot