Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

timers test: unref does not return this #9171

Closed
wants to merge 1 commit into from
Closed

timers test: unref does not return this #9171

wants to merge 1 commit into from

Conversation

jscissr
Copy link

@jscissr jscissr commented Feb 9, 2015

The unref() call returns undefined, not this.
The test already worked before, because the interval was still unref'd,
and the test also succeeds without clearing the interval.

The unref() call returns undefined, not this.
The test already worked before, because the interval was still unref'd,
and the test also succeeds without clearing the interval.
@cjihrig
Copy link

cjihrig commented Feb 9, 2015

LGTM

@tjfontaine
Copy link

LGTM, though the commit message would be slightly better as just test: timers-unref ...

@jscissr
Copy link
Author

jscissr commented Feb 9, 2015

Can/should I do something to fix the commit message?

@cjihrig
Copy link

cjihrig commented Feb 9, 2015

@jscissr you can do a git commit --amend to change the commit message and then git push --force it. Otherwise, it can be changed during landing.

@jscissr
Copy link
Author

jscissr commented Feb 9, 2015

I actually only used the github.com webinterface to change and commit, so I think the first option wouldn't work.

trevnorris pushed a commit that referenced this pull request Feb 11, 2015
Timeout#unref() call returns undefined, not this. The test already
worked before, because the interval was still unref'd, and the test also
succeeds without clearing the interval.

PR-URL: #9171
Reviewed-by: Colin Ihrig <[email protected]>
Reviewed-by: Timothy J Fontaine <[email protected]>
@trevnorris
Copy link

Thanks. Landed in d2b5461.

@trevnorris trevnorris closed this Feb 11, 2015
@jscissr jscissr deleted the patch-1 branch February 11, 2015 22:11
cjihrig pushed a commit to nodejs/node that referenced this pull request Feb 13, 2015
Timeout#unref() call returns undefined, not this. The test already
worked before, because the interval was still unref'd, and the test also
succeeds without clearing the interval.

PR-URL: nodejs/node-v0.x-archive#9171
Reviewed-by: Colin Ihrig <[email protected]>
Reviewed-by: Timothy J Fontaine <[email protected]>

Conflicts:
	test/simple/test-timers-unref.js
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants