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

skip not covered lines on node master #92

Merged
merged 1 commit into from Feb 10, 2018

Conversation

@mcollina
Copy link
Contributor

mcollina commented Feb 9, 2018

Alternative fix for the code coverage in master/node 10. See #91.

@@ -120,13 +120,15 @@ internals.Request.prototype._read = function (size) {

setImmediate(() => {

/* $lab:coverage:off$ */
if (this._shot.isDone) {
if (this._shot.simulate.end !== false) { // 'end' defaults to true

This comment has been minimized.

Copy link
@mtharrison

mtharrison Feb 10, 2018

Member

Can you wrap only this inner if statement please? We should have coverage on the outer if

@mcollina mcollina force-pushed the mcollina:skip-not-covered-lines branch from 524dd97 to cc7d071 Feb 10, 2018
@mcollina

This comment has been minimized.

Copy link
Contributor Author

mcollina commented Feb 10, 2018

@mtharrison updated.

@mtharrison mtharrison added the test label Feb 10, 2018
@mtharrison mtharrison merged commit 75ce1a0 into hapijs:master Feb 10, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Feb 12, 2018

Why is this not covered in node 10?

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Feb 12, 2018

I guess _read() isn't called twice whereas it was node < 10. A change to how streams work?

@mcollina

This comment has been minimized.

Copy link
Contributor Author

mcollina commented Feb 12, 2018

Yes. Before, _read() was called for every asynchronous push(). Now it’s called only once.

(I think we can all agree that those lines are not very intuitive and this is a good change. It also fixes a bad bug with object streams).

@mcollina

This comment has been minimized.

Copy link
Contributor Author

mcollina commented Feb 15, 2018

When do you plan to make a shot release with this fix? in CITGM we pull from npm.

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Feb 15, 2018

I can do one right now, assumed it was using master.

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Feb 15, 2018

v4.0.5 released

@mtharrison mtharrison added this to the 4.0.5 milestone Feb 15, 2018
@mtharrison mtharrison self-assigned this Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.