Implement Object.getOwnPropertyNames and Object.getOwnPropertySymbols #1606
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Conformance testsBeforeResults: AfterTotal tests: 80930 |
jedel1043
requested changes
Oct 1, 2021
Test262 conformance changes:
Fixed tests (121): |
Smaller diffs are better for first contributions. This allows us to review and merge ASAP.
Please do so! We are currently lacking a lot of tests so they are always appreciated :) |
|
Thank you very much for the contribution!! It's interesting how |
|
@jedel1043 Thanks a lot for helping review this!
Sure, sounds great 👍
Alright, I'll write some soon! |
@Razican no problem!
Indeed, I was wondering about the same thing... I don't think the following provides much value since it seems that boa tries to stick to the spec as much as possible, but when running the following code on the browser (I use firefox): const returned = "Array,BigInt,Boolean,Date,Error,EvalError,Function,Infinity,JSON,Map,Math,NaN,Number,Object,RangeError,ReferenceError,Reflect,RegExp,Set,String,Symbol,SyntaxError,TypeError,URIError,console,globalThis,isFinite,isNaN,parseFloat,parseInt,undefined".split(",");
for (let ret of returned) {
if (!Object.getOwnPropertyNames(this).includes(ret)) {
console.log("not included:", ret);
}
}I don't see any output in the console, which seems to suggest that boa's |
@jedel1043 just curious, is there a way for me to recreate this output easily? |
|
Yeah, the workflow has an "EcmaScript official test suite" check with exactly that. For first contributions we have to manually run the workflow but after the first contribution you can just check that section to see the number of fixed and broken tests |
jedel1043
added
builtins
|
The "update comment" job of that check gives you a nifty table that you can copy-paste as a comment: Test262 conformance changes:
Fixed tests (121): |
Ah okay got it, thanks! |
jedel1043
requested changes
Oct 2, 2021
jedel1043
reviewed
Oct 2, 2021
jedel1043
approved these changes
Oct 2, 2021
|
We normally wait for two reviews in order to merge a PR, just in case the first reviewer missed something, so you just need to wait a bit of time until another maintainer approves it and it should be merged asap :) |
Sure, no problem. Thanks for the help! |
Razican
requested review from
Razican,
jasonwilliams,
HalidOdat,
RageKnify and
0x7D2B
Razican
approved these changes
Oct 2, 2021
jedel1043
added
the
Hacktoberfest
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Scope
This Pull Request partially fixes/closes #1580.
In this PR, I implemented
Object.getOwnPropertyNamesandObject.getOwnPropertySymbols. Originally I planned to open separate PRs for each of the missing method listed in #1580, but ended up implementing these two methods in one PR since they are very much alike.I still plan to work on
Object.fromEntriesandObject.hasOwnin separate PRs, though. Please let me know in case it's preferred to have the implementation of these four methods in this PR instead!Testing
There are several failing conformance tests, which are due to the following:
Proxynot defined (I'm guessing it's not implemented yet?)Object.getOwnPropertyNames(this)returning slightly different set of properties from expected:Array,Boolean,Date,Date,Error,EvalError,Function,Infinity,JSON,Math,NaN,Number,Object,RangeError,ReferenceError,RegExp,String,SyntaxError,TypeError,URIError,decodeURI,decodeURIComponent,encodeURI,encodeURIComponent,eval,isFinite,isNaN,parseFloat,parseInt,undefinedArray,BigInt,Boolean,Date,Error,EvalError,Function,Infinity,JSON,Map,Math,NaN,Number,Object,RangeError,ReferenceError,Reflect,RegExp,Set,String,Symbol,SyntaxError,TypeError,URIError,console,globalThis,isFinite,isNaN,parseFloat,parseInt,undefinedtest/built-ins/Object/getOwnPropertySymbols/order-after-define-property.jsfailing withUncaught "TypeError": "can't convert symbol to string". I think this has something to do with calling[Symbol("a")].toString(), but I'm not exactly sure. Anyhow, I tried editing the test such that no string conversion happens, and the test passed.On another note, should I also write some custom tests in the
builtins/object/tests.rsfile?