Skip to content

Conversation

calvinmetcalf
Copy link
Contributor

and there is a new es6 feature being used in tests and a couple more fixtures!

@calvinmetcalf
Copy link
Contributor Author

and a few tests that use encodings not all versions of node have, no actual changes to streams here so no rush

@Fishrock123 Fishrock123 changed the title fist run at 6.4.0 first run at 6.4.0 Aug 22, 2016
@calvinmetcalf
Copy link
Contributor Author

ok the issue with the preprocessing test was basically it was relying on some specific tty stuff that was kinda tangential to streams specifically

@calvinmetcalf
Copy link
Contributor Author

if others are on the same page about being able to ditch that preprocess test I can merge this

@mcollina
Copy link
Member

@calvinmetcalf LGTM

As a side note, that test is not targeting readable-stream, as it is using fs, readline and others`, so there is nothing we can fix here.

Can we avoid pulling it the next time? That test also relies on two txt files, those should be removed as well. It's really a test about fs and readline rather than streams.

As a side note, that test it was introduced by @vsemozhetbyt in nodejs/node#7741 and nodejs/node@ea725ed

cc @addaleax

@calvinmetcalf
Copy link
Contributor Author

oh right need to remove those

@calvinmetcalf
Copy link
Contributor Author

we'd avoid pulling it in if it wasn't named 'stream' it's just a function of the fact that it was touching 3 diff modules and it had to pick one

@mcollina
Copy link
Member

@calvinmetcalf we should move it then.

@addaleax
Copy link
Member

I’m not sure what the process for pulling in tests is but if moving that one test in the node repo makes anything easier for anyone that shouldn’t be a problem?

@calvinmetcalf
Copy link
Contributor Author

not a big deal any more, I just put something in the code that doesn't pull
it in

On Wed, Aug 31, 2016 at 3:11 PM Anna Henningsen [email protected]
wrote:

I’m not sure what the process for pulling in tests is but if moving that
one test in the node repo makes anything easier for anyone that shouldn’t
be a problem?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#225 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABE4n1fsktxGhufYo5JvHPu_77t_-Clfks5qldHugaJpZM4Jm1kx
.

@calvinmetcalf
Copy link
Contributor Author

this is a transient error, I'll plan on merging this latter if nobody objects

@calvinmetcalf calvinmetcalf merged commit 3e8da33 into master Sep 6, 2016
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.

3 participants