This repository has been archived by the owner. It is now read-only.

Tests for joyent/node#5141 not working as advertised #5183

Closed
arhart opened this Issue Mar 31, 2013 · 2 comments

Comments

Projects
None yet
2 participants

arhart commented Mar 31, 2013

The tests for #5141 do not agree with the test comments. Of particular concern, first and second both test the !state.readign case, meaning second misses the state.reading && state.length case.

In other words,

} else if (state.length) {
  emitReadable(this, state);

can be removed from Readable.prototype.on and the tests would not fail.

I'll try to work up a pull request, but this'll be my first time so it might be awhile.
I've not yet submitted the CLA.

arhart added a commit to arhart/node that referenced this issue Mar 31, 2013

test: test intended code-paths
The tests did not agree with the test comments. Tests first and second
were both testing the !state.reading case. Now second tests the
state.reading && state.length case.

Fixes nodejs/node-v0.x-archive#5183

arhart commented Mar 31, 2013

CLA submitted

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Apr 1, 2013

test: test intended code-paths
The tests did not agree with the test comments. Tests first and second
were both testing the !state.reading case. Now second tests the
state.reading && state.length case.

Fixes nodejs/node-v0.x-archive#5183

isaacs commented Apr 1, 2013

Landed on db8ce89. Thanks!

@isaacs isaacs closed this Apr 1, 2013

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.