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

Modify Object.prototype in the tests #179

Merged
merged 1 commit into from
Dec 19, 2013
Merged

Modify Object.prototype in the tests #179

merged 1 commit into from
Dec 19, 2013

Conversation

mathiasbynens
Copy link
Contributor

Ref. #175, #177.

@ljharb
Copy link
Collaborator

ljharb commented Dec 19, 2013

I'd prefer these to be explicit tests that set and delete the Object.prototype values, rather than setting them for all tests blindly.

@mathiasbynens
Copy link
Contributor Author

I disagree. Might as well apply it to all tests in those files, in case some new tests (added later on) highlight similar bugs in other parts of the code.

@ljharb
Copy link
Collaborator

ljharb commented Dec 19, 2013

What happens if the presence of something extra on Object.prototype masks an improper test for something new? If you want to run the tests twice, once with the modifications and once without, that would allay my concerns, but only running tests in an environment where Object.prototype is modified (a very unlikely, hugely discouraged environment in the first place) is going to reflect the minority case, not the majority.

@mathiasbynens
Copy link
Contributor Author

What happens if the presence of something extra on Object.prototype masks an improper test for something new?

IMHO that seems way more far-fetched than something breaking because of Object.prototype being extended.

@ljharb
Copy link
Collaborator

ljharb commented Dec 19, 2013

The whole premise of these tests and the previous patch is something breaking because Object.prototype was extended. I think it being extended in the first place is insanely far-fetched - but I liked that the argument checking was more explicit anyways and it just so happened to fix the problem. I'll be happy to work on another PR with tight, explicit tests for this behavior for the affected functions if you'd rather not, but running tests in an unlikely and "bad practice" environment isn't a great idea imo.

@mathiasbynens
Copy link
Contributor Author

The whole premise of these tests and the previous patch is something breaking because Object.prototype was extended.

Yeah, my point exactly.

@ljharb
Copy link
Collaborator

ljharb commented Dec 19, 2013

So anything that's at risk of breaking when it's extended needs to be tested both when it is extended, and when it's not - otherwise we risk a regression in a pristine environment. Either likelihood is irrelevant and all possibilities need to be accounted for, or likelihood matters and we don't need to concern ourselves with people who modify Object.prototype, since that basically never happens.

@mathiasbynens
Copy link
Contributor Author

So anything that's at risk of breaking when it's extended needs to be tested both when it is extended, and when it's not

Everything is at risk of breaking when natives are extended. That’s why running all tests in such an environment is usually a good idea. The chance of some legitimate bug being masked by an extended native is extremely small — much smaller than the possibility of running code in a tainted environment, but I’m fine with running the tests in a pristine environment in addition to that if it helps you sleep at night ;)

paulmillr added a commit that referenced this pull request Dec 19, 2013
Modify `Object.prototype` in the tests
@paulmillr paulmillr merged commit a2893b6 into paulmillr:master Dec 19, 2013
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