Skip to content

test: remove internal headers from addons - #7947

Closed
gibfahn wants to merge 1 commit into
nodejs:masterfrom
gibfahn:pr-openssl-addon
Closed

test: remove internal headers from addons#7947
gibfahn wants to merge 1 commit into
nodejs:masterfrom
gibfahn:pr-openssl-addon

Conversation

@gibfahn

@gibfahngibfahn commented Aug 2, 2016

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test-addons (UNIX), or vcbuild test-addons nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test, addons

Description of change

Continuation of 3c85f4e (in #6734). Make the addons more like userland ones.

@bnoordhuis not sure whether util.h and util-inl.h were being used in openssl-binding/binding.cc. The asserts in test.js still seem to pass.

EDIT: CI is green

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Aug 2, 2016
@mscdex

Copy link
Copy Markdown
Contributor

@mscdexmscdex added the addons Issues and PRs related to native addons. label Aug 2, 2016
@gibfahn

Copy link
Copy Markdown
MemberAuthor

@mscdex Whoops, looks like I removed one line too many. If you could rerun the CI that'd be great.

@mscdex

Copy link
Copy Markdown
Contributor

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

@cjihrig

Copy link
Copy Markdown
Contributor

The last CI run was green everywhere but FreeBSD. Trying again: https://ci.nodejs.org/job/node-test-pull-request/3505/

LGTM if it comes back green. I guess util.h and util-inl.h weren't being used.

@gibfahn

gibfahn commented Aug 2, 2016

Copy link
Copy Markdown
MemberAuthor

FreeBSD failure was this (probaby unrelated):

not ok 229 parallel/test-dgram-send-empty-array
# /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-dgram-send-empty-array.js:15
# throw new Error('Timeout');
# ^
# # Error: Timeout
# at Timeout._onTimeout (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-dgram-send-empty-array.js:15:9)
# at tryOnTimeout (timers.js:232:11)
# at Timer.listOnTimeout (timers.js:202:5)
---
duration_ms: 0.484
...

@gibfahn

Copy link
Copy Markdown
MemberAuthor

3rd CI came back green, thanks @cjihrig

@gibfahn

Copy link
Copy Markdown
MemberAuthor

@bnoordhuis I'd appreciate a review from you as I'm changing your code.

@bnoordhuis

Copy link
Copy Markdown
Member

LGTM

1 similar comment
@jasnell

Copy link
Copy Markdown
Member

LGTM

jasnell pushed a commit that referenced this pull request Aug 5, 2016
PR-URL: #7947
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

Copy link
Copy Markdown
Member

Landed in 9a0d26f! Thank you!

@jasnell

Copy link
Copy Markdown
Member

Whoops, forgot to close when landing...

@jasnelljasnell closed this Aug 8, 2016
@cjihrigcjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
PR-URL: #7947
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrigcjihrig mentioned this pull request Aug 11, 2016
@MylesBorins

Copy link
Copy Markdown
Contributor

neither of these files live in v4.x, adding dont land

@gibfahn
gibfahn deleted the pr-openssl-addon branch December 16, 2016 11:15
MylesBorins pushed a commit that referenced this pull request Nov 2, 2017
Backport-PR-URL: #16550
PR-URL: #7947
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Nov 3, 2017
@refackrefack mentioned this pull request Oct 18, 2018
3 tasks
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.

7 participants

@gibfahn@mscdex@cjihrig@bnoordhuis@jasnell@MylesBorins@nodejs-github-bot