Skip to content

test: add new.target add-on regression test - #9689

Merged
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:new-target-addon-test
Nov 21, 2016
Merged

test: add new.target add-on regression test#9689
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:new-target-addon-test

Conversation

@bnoordhuis

@bnoordhuisbnoordhuis commented Nov 18, 2016

Copy link
Copy Markdown
Member

Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.

Refs: #9288
Refs: #9293

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

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Nov 18, 2016
@addaleaxaddaleax added the addons Issues and PRs related to native addons. label Nov 18, 2016

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment threadtest/addons/new-target/test.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a suggestion but maybe check new Class() instanceof binding.Class, too?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Good idea, done. Also fixed the lint warning.

Comment threadtest/addons/new-target/binding.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The linter seems to want anonymous namespace :P

Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.
PR-URL: nodejs#9689
Refs: nodejs#9288
Refs: nodejs#9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@bnoordhuis
bnoordhuis deleted the new-target-addon-test branch November 21, 2016 13:37
@bnoordhuis
bnoordhuis merged commit 13c4f44 into nodejs:masterNov 21, 2016
addaleax pushed a commit that referenced this pull request Nov 22, 2016
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.
PR-URL: #9689
Refs: #9288
Refs: #9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins

Copy link
Copy Markdown
Contributor

@bnoordhuis this is causing failures. Can you manually backport?

MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.
PR-URL: #9689
Refs: #9288
Refs: #9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax

Copy link
Copy Markdown
Member

@thealphanerd The v6.x backport is at #9293 (so I’m removing lts-watch-v6.x) and I am not sure this is feasible for v4.x at all

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.
PR-URL: #9689
Refs: #9288
Refs: #9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorinsMylesBorins mentioned this pull request Dec 21, 2016
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Mar 2, 2017
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.
PR-URL: nodejs#9689
Refs: nodejs#9288
Refs: nodejs#9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.
PR-URL: #9689
Refs: #9288
Refs: #9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorinsMylesBorins mentioned this pull request Mar 9, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addonsIssues and PRs related to native addons.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@bnoordhuis@MylesBorins@addaleax@nodejs-github-bot