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

test: remove arguments.callee usage #3167

Closed
wants to merge 1 commit into from

Conversation

@silverwind
Copy link
Contributor

commented Oct 3, 2015

arguments.callee is forbidden in strict mode and the fact that it's being used masked a possible error in this test like in this case:

https://ci.nodejs.org/job/node-test-binary-arm/101/RUN_SUBSET=0,nodes=pi1-raspbian-wheezy/tapTestReport/test.tap-39/

test: remove arguments.callee usage
arguments.callee is forbidden in strict mode and the fact that it's
being used masked a possible error in this test.

@silverwind silverwind added the test label Oct 3, 2015

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2015

Change LGTM, I am okay if the CI is okay.

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2015

CI: https://ci.nodejs.org/job/node-test-pull-request/412/

For the record, this change should've been done when we turned all tests into strict mode, pretty much an oversight on my part.

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2015

@silverwind Mmmm yeah. But how it was not failing the builds till now?

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2015

@thefourtheye because it only errors when it reaches a failure condition. If the test passes, the strict mode warning is never reached. What we have here, is a masked error on the linked test.

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2015

FWIW: this is the only remaining use of callee / caller in all of lib and test.

@targos

This comment has been minimized.

Copy link
Member

commented Oct 3, 2015

LGTM, failures are unrelated.

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Oct 3, 2015

LGTM

silverwind added a commit that referenced this pull request Oct 5, 2015
test: remove arguments.callee usage
arguments.callee is forbidden in strict mode and the fact that it's
being used masked a possible error in this test.

PR-URL: #3167
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2015

Thanks, landed in 342c3a1.

@silverwind silverwind closed this Oct 5, 2015

@jasnell jasnell referenced this pull request Oct 8, 2015
29 of 29 tasks complete
silverwind added a commit that referenced this pull request Oct 8, 2015
test: remove arguments.callee usage
arguments.callee is forbidden in strict mode and the fact that it's
being used masked a possible error in this test.

PR-URL: #3167
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.