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

Add tests for Object.hasOwn #2995

Merged
merged 10 commits into from
Jun 16, 2021
Merged

Add tests for Object.hasOwn #2995

merged 10 commits into from
Jun 16, 2021

Conversation

jamiebuilds
Copy link
Member

@jamiebuilds jamiebuilds commented May 25, 2021

Proposal: https://github.com/tc39/proposal-accessible-object-hasownproperty

This proposal just reached stage 3, its not currently published in the draft but I've used esid: sec-object.hasown since that's what it's very likely going to end up being.

This PR is largely a copy-and-paste job from the old Object.prototype.hasOwnProperty tests, but I've tried to modernize anything I can.

ljharb
ljharb previously requested changes May 25, 2021
test/built-ins/Object/hasOwn/length.js Outdated Show resolved Hide resolved
test/built-ins/Object/hasOwn/name.js Outdated Show resolved Hide resolved
jamiebuilds and others added 3 commits May 25, 2021 14:18
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I might have missed it while reading the tests, but I think we are not testing the property descriptor of Object.hasOwn (something like https://github.com/tc39/test262/blob/main/test/built-ins/Object/assign/assign-descriptor.js).

@ljharb ljharb dismissed their stale review May 25, 2021 21:26

feedback addressed

@jamiebuilds
Copy link
Member Author

@nicolo-ribaudo nope didn't have that, added

Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

Thanks for contributing these tests, Jamie!

Copy link
Contributor

@rwaldron rwaldron left a comment

Choose a reason for hiding this comment

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

There's a lot of test material here, which I generally really appreciate, but there are things being tested that aren't relevant to property "own"-ness.

test/built-ins/Object/hasOwn/hasown.js Outdated Show resolved Hide resolved
@jamiebuilds
Copy link
Member Author

There's a lot of test material here, which I generally really appreciate, but there are things being tested that aren't relevant to property "own"-ness.

I copied I think all of these tests from test/built-ins/Object/prototype/hasOwnProperty, I'm not entirely sure which ones should be removed.

@rwaldron
Copy link
Contributor

Well, then it seems that directory is filled with tests that are irrelevant to "own"-ness. We can deal with that later.

@rwaldron
Copy link
Contributor

Everyone that's reviewed should come back and resolve or re-review for @jamiebuilds

rwaldron
rwaldron previously approved these changes May 29, 2021
@codehag
Copy link
Contributor

codehag commented Jun 1, 2021

Pinging this, in the hopes that it gets the reviews updated. @jugglinmike i think yours is the only request changes rn?

Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

We're close! It just looks like a couple small bugs slipped in through the latest refactoring.

test/built-ins/Object/hasOwn/hasown.js Outdated Show resolved Hide resolved
test/built-ins/Object/hasOwn/prototype.js Outdated Show resolved Hide resolved
@jamiebuilds
Copy link
Member Author

Okay @jugglinmike I've updated those test cases, this should now be good to go

Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

Looks good to me! Nice work :)

By the way, the CI failures are about failing tests. We recently configured the system to tolerate test failures, so my guess is that this was caused by the base branch being behind the upstream main branch.

@jugglinmike jugglinmike merged commit bad7c04 into tc39:main Jun 16, 2021
@jamiebuilds jamiebuilds deleted the object-hasown branch June 16, 2021 21:35
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.

None yet

6 participants