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

Reduce reliance on fnGlobalObject.js #595

Closed
wants to merge 3 commits into from

Conversation

jugglinmike
Copy link
Contributor

This harness function is not necessary in the majority of cases in which
it is used. Remove its usage to simplify tests and decrease the amount
of domain-specific knowledge necessary to contribute to the test suite.

Persist the harness function itself for use by future tests for ES2015
modules (such a helper is necessary for tests that are interpreted as
module code).

This resolves gh-568

This harness function is not necessary in the majority of cases in which
it is used. Remove its usage to simplify tests and decrease the amount
of domain-specific knowledge necessary to contribute to the test suite.

Persist the harness function itself for use by future tests for ES2015
modules (such a helper is necessary for tests that are interpreted as
module code).
---*/

assert.sameValue(Array.isArray(fnGlobalObject()), false, 'Array.isArray(fnGlobalObject())');
assert.sameValue(Array.isArray(this), false, 'Array.isArray(fnGlobalObject())');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the assertion message still has the old method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops! I'll update all three assertion messages

@leobalter
Copy link
Member

a few comments above, the current tests are looking good.

we can delete harness/fnGlobalObject.js as well, right?

There are some cases declaring a oldLen to store global.length variable which is totally unnecessary. This is not the focus of this PR, but if it's not handled here, they should be removed on a follow up patch. See #595 (diff)

There are some other tests requiring some updates for good. Once again, not in this PR. See #595 (diff)

@jugglinmike
Copy link
Contributor Author

we can delete harness/fnGlobalObject.js as well, right?

I'd rather not--the utility function is still useful for module code, where
the global this is undefined. I tried to document this concern in the
commit message, but I'd be happy to re-word that if you think it could be more
clear.

I agree that it would be good to clean these tests up, and also that it is
better left for another patch.

Thanks for the review!

@leobalter
Copy link
Member

the utility function is still useful for module code

That's right, I forgot about it.

Persist the harness function itself for use by future tests for ES2015
modules (such a helper is necessary for tests that are interpreted as
module code).

My bad I started the code review minutes after I opened this PR (locally first). This message is great.

@leobalter
Copy link
Member

LGTM

@goyakin
Copy link
Member

goyakin commented Apr 25, 2016

Merged as 7f88f29.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reducing reliance on fnGlobalObject.js
3 participants