GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: remove internal headers from addons by gibfahn · Pull Request #7947 · nodejs/node

Conversation

@gibfahn

@gibfahn gibfahn commented Aug 2, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Aug 2, 2016
@mscdex

mscdex commented Aug 2, 2016

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/3501/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex mscdex added the addons Issues and PRs related to native addons. label Aug 2, 2016
@gibfahn
test: remove internal headers from addons
4ef5c85
@gibfahn
gibfahn force-pushed the pr-openssl-addon branch from a7eaf5d to 4ef5c85 Compare August 2, 2016 11:34
@gibfahn

gibfahn commented Aug 2, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Aug 2, 2016

Copy link
Copy Markdown
Contributor

CI again: https://ci.nodejs.org/job/node-test-pull-request/3502/
gibfahn reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Aug 2, 2016

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn

gibfahn commented Aug 2, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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
  ...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn

gibfahn commented Aug 3, 2016

Copy link
Copy Markdown
Member Author

3rd CI came back green, thanks @cjihrig
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn

gibfahn commented Aug 3, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Aug 3, 2016

Copy link
Copy Markdown
Member

LGTM
gibfahn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1 similar comment
@jasnell

jasnell commented Aug 3, 2016

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell pushed a commit that referenced this pull request Aug 5, 2016
@gibfahn @jasnell
test: remove internal headers from addons
9a0d26f 
PR-URL: #7947
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell

jasnell commented Aug 5, 2016

Copy link
Copy Markdown
Member

Landed in 9a0d26f! Thank you!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Aug 8, 2016

Copy link
Copy Markdown
Member

Whoops, forgot to close when landing...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Aug 8, 2016
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
v6.4.0 Release Proposal #8020
Closed
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
@gibfahn @cjihrig
test: remove internal headers from addons
b5beae2 
PR-URL: #7947
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
v6.4.0 Proposal #8070
Merged
@MylesBorins MylesBorins added the lts-watch-v4.x label Sep 30, 2016
@MylesBorins MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Oct 11, 2016
@MylesBorins

MylesBorins commented Oct 11, 2016

Copy link
Copy Markdown
Contributor

neither of these files live in v4.x, adding dont land
gibfahn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn
gibfahn deleted the pr-openssl-addon branch December 16, 2016 11:15
@gibfahn gibfahn mentioned this pull request Oct 27, 2017
[v4.x backport] Use external headers in addons tests #16550
Closed
2 tasks done
MylesBorins pushed a commit that referenced this pull request Nov 2, 2017
@gibfahn @MylesBorins
test: remove internal headers from addons
cb92f93 
Backport-PR-URL: #16550
PR-URL: #7947
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
v4.8.6 proposal #16500
Merged
@refack refack mentioned this pull request Oct 18, 2018
build: spawn make test-ci with -j1 #23733
Merged
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

addons Issues and PRs related to native addons. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@gibfahn @mscdex @cjihrig @bnoordhuis @jasnell @MylesBorins @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.