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

test: add an zlib binding addon test #8039

Closed
wants to merge 1 commit into from
Closed

test: add an zlib binding addon test #8039

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 9, 2016 •
edited
Loading

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

test/zlib

Description of change

Add a test addon that makes use of the zlib implementation bundled with node, checking that a compression/decompression round-trip works.

This is largely based on the already-existing OpenSSL addon.

Fixes: #7535

Initial CI: https://ci.nodejs.org/job/node-test-commit/4488/

@addaleax
test: add an zlib binding addon test
01b42cf 
Add a test addon that makes use of the zlib implementation bundled
with node, checking that a compression/decompression round-trip works.

This is largely based on the already-existing OpenSSL addon.

Fixes: nodejs#7535
@addaleax addaleax added zlib Issues and PRs related to the zlib subsystem. test Issues and PRs related to the tests. addons Issues and PRs related to native addons. labels Aug 9, 2016
@jasnell
Copy link
Member

jasnell commented Aug 9, 2016

That was fast!

@jasnell
Copy link
Member

jasnell commented Aug 9, 2016

LGTM if CI is green.

@addaleax
Copy link
Member Author

addaleax commented Aug 9, 2016

CI is actually green, with one unrelated Windows test failure.

@ghost
Copy link

ghost commented Aug 10, 2016

Smart test. Thanks for fixing this.

@bnoordhuis
Copy link
Member

LGTM. Unrelated failure on the 2,vs2015,win2012r2 buildbot.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 10, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Aug 12, 2016

New C since there were a couple (apparently unrelated) failures in the last run: https://ci.nodejs.org/job/node-test-pull-request/3647/

@jasnell
Copy link
Member

jasnell commented Aug 12, 2016

more unrelated build bot failures in CI :-( ... few tests still pending tho.

@addaleax
Copy link
Member Author

This time, apparently the binding used in the test here failed to be created on CI, although that went perfectly fine in the previous CI, and I can’t see anything hinting at the cause of that… /cc @nodejs/build?

@addaleax
Copy link
Member Author

Trying CI once more: https://ci.nodejs.org/job/node-test-commit/4594/

@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

CI is green. Landing

jasnell pushed a commit that referenced this pull request Aug 18, 2016
@addaleax @jasnell
test: add an zlib binding addon test
4c9b0bd 
Add a test addon that makes use of the zlib implementation bundled
with node, checking that a compression/decompression round-trip works.

This is largely based on the already-existing OpenSSL addon.

Fixes: #7535
PR-URL: #8039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

Landed in 4c9b0bd

@jasnell jasnell closed this Aug 18, 2016
@addaleax addaleax deleted the zlib-addon-test branch August 18, 2016 17:46
evanlucas pushed a commit that referenced this pull request Aug 20, 2016
@addaleax @evanlucas
test: add an zlib binding addon test
d401e55 
Add a test addon that makes use of the zlib implementation bundled
with node, checking that a compression/decompression round-trip works.

This is largely based on the already-existing OpenSSL addon.

Fixes: #7535
PR-URL: #8039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@evanlucas evanlucas mentioned this pull request Aug 24, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
@addaleax
test: add an zlib binding addon test
5fb8978 
Add a test addon that makes use of the zlib implementation bundled
with node, checking that a compression/decompression round-trip works.

This is largely based on the already-existing OpenSSL addon.

Fixes: #7535
PR-URL: #8039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

@addaleax it looks like this is causing failures on windows on v4.x

https://ci.nodejs.org/job/node-test-binary-windows/RUN_SUBSET=1,VS_VERSION=vs2013,label=win2008r2/4279/tapTestReport/test.tap-259/

not ok 259 addons/zlib-binding/test
# module.js:327
# throw err;
# ^
# 
# Error: Cannot find module './build/Release/binding'
# at Function.Module._resolveFilename (module.js:325:15)
# at Function.Module._load (module.js:276:25)
# at Module.require (module.js:353:17)
# at require (internal/module.js:12:17)
# at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vs2013\label\win2008r2\test\addons\zlib-binding\test.js:6:17)
# at Module._compile (module.js:409:26)
# at Object.Module._extensions..js (module.js:416:10)
# at Module.load (module.js:343:32)
# at Function.Module._load (module.js:300:12)
# at Function.Module.runMain (module.js:441:10)

I'm going to avoid backing it out for right now as I'm landing PR's and don't want to cause more rebases... but will be looking at backing it out after a few things land. If you could look into it and find a solution before then that would be rad!

@ghost
Copy link

ghost commented Oct 10, 2016 •
edited by ghost
Loading

It is supposed to fail under Windows (4.x) since the fix of zlib for Windows is only applied to the 6.x of Node.js.

@MylesBorins
Copy link
Contributor

would you be willing to submit a pr to v4.x that sets it as flaky? or would
we be better not backporting

On Mon, Oct 10, 2016, 2:16 PM Alex Hultman notifications@github.com wrote:

It is supposed to fail under Windows since the fix of zlib for Windows is
only applied to the 6.x of Node.js.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#8039 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecV2nnmzpG_kuz6X0PM4epCtP7aVp1ks5qyoDxgaJpZM4JgmYK
.

@ghost
Copy link

ghost commented Oct 10, 2016

I think the idea was to not backport any of those fixes (zlib, openssl) to 4.x since some addons have kind of ignored to really fix the real issue and instead do link to their own versions of openssl and zlib. So when 6.x added its own exposure of zlib and openssl some addons broke because of colliding symbols or something like that.

@addaleax
Copy link
Member Author

Yeah, if this is backported to v4, the test should probably be marked as flaky.

@MylesBorins
Copy link
Contributor

Im going to go ahead and back this out of v4.x. Please feel free to submit a backport with the flaky tst set appropriately, but I don't think we need it

@richardlau richardlau mentioned this pull request Dec 5, 2016
Closed
@richardlau richardlau mentioned this pull request Dec 15, 2016
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. test Issues and PRs related to the tests. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@addaleax @jasnell @bnoordhuis @cjihrig @MylesBorins