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.

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

Copy over node buffer tests #51

Merged
merged 15 commits into from
Dec 24, 2014

Conversation

jessetane
Copy link
Collaborator

These patches pull in node's buffer tests and make the changes required to pass them.

This includes a proper fix for #48 /cc @trollixx.

@jessetane jessetane force-pushed the copy-over-node-buffer-tests branch from 3945e4c to 767070e Compare December 22, 2014 22:52
@feross feross merged commit 767070e into feross:master Dec 24, 2014
@feross
Copy link
Owner

feross commented Dec 24, 2014

This is an incredible PR! You are awesome @jessetane!

Will release 3.0.0 tonight, because this PR contains many breaking changes (for the better):

  • toString('ascii') and slice('ascii') work correctly (strip off high order bits)
  • correct exception types thrown (RangeError instead of TypeError in many places)
  • do not encode partial code units to utf16
  • do not encode partial or invalid code points to utf8
  • copy() returns length

@feross
Copy link
Owner

feross commented Dec 24, 2014

I just added you as a github collaborator! Please follow the OPEN Open Source guidelines :) And thanks for being awesome!

@jessetane
Copy link
Collaborator Author

Very cool, glad to be able to help out!

It looks like the tests may be taking too long after all? I can't immediately tell:

https://s3.amazonaws.com/archive.travis-ci.org/jobs/44993014/log.txt

If need be, the node tests can be downloaded without transforming to use tape and they will run almost instantly with assert - tape output would be sacrificed, and a single failed test would crash the whole process, but they would be fast...

@feross
Copy link
Owner

feross commented Dec 24, 2014

Looks like it. I don't think tap output is super important for the node tests, and they do cause a lot of traffic from the browsers (sauce labs) back to the test harness (travis) which increases the flakiness of the tests.

Were you able to run the zuul tests locally? You can make an open source sauce labs account and follow the instructions here: https://github.com/defunctzombie/zuul/wiki/Cloud-testing

@feross
Copy link
Owner

feross commented Dec 24, 2014

This might be hard to do as a transform, but leaving the tight loops as asserts and just inserting a single t.pass('xxxx test passed') message at the end would be a good balance between informational test output and speed.

@jessetane
Copy link
Collaborator Author

I tried running the tests in local browsers but not at sauce, lemme try that real quick.

@feross
Copy link
Owner

feross commented Dec 24, 2014

Okay, 3.0.0 is released now.

@jessetane
Copy link
Collaborator Author

w00t!

@jessetane jessetane deleted the copy-over-node-buffer-tests branch December 24, 2014 19:50
@feross
Copy link
Owner

feross commented Dec 31, 2014

@jessetane I just merged your base64-js PR (beatgammit/base64-js#10). Could you send a PR to buffer to remove the special handling you added here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants