-
Notifications
You must be signed in to change notification settings - Fork 512
Add upsert tests #4454
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 upsert tests #4454
Conversation
These tests are currently being executed in SpiderMonkey as `non262` tests, and were exported using the `test262-export.py` script. Linting errors were then corrected.
test/staging/upsert/Map/getOrInsert/append-new-values-normalizes-zero-key.js
Outdated
Show resolved
Hide resolved
ptomato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This is staging, so you should feel free to (and already have the appropriate permissions to) commit these as long as they are executable by other engines' test262 harnesses.
That said, I suspect they will not pass in any other engines unless you add includes: [sm/non262.js, sm/non262-shell.js] to each one.
Optionally, there's also some low-hanging fruit that could make them slightly more ready to move out of staging when the time comes:
- Add a feature for the upsert proposal to
features.txtand add it tofeaturesin the frontmatter of each test - Use
verifyPropertyinstead ofassert.deepEqual(Object.getOwnPropertyDescriptor(...etc)) - Use
assert.throws(ExceptionType, function () { ... })instead ofassertThrowsInstanceOf(function () { ... }, ExceptionType)
|
Thanks for the suggestions on how to fix these up, I'll look into that next week prior to committing. |
I haven't seen it tested before in test262, but if you want you could restore those tests using |
|
@ptomato Hi Philip, I've addressed your suggestions and checked that the tests pass in engine262 (with my upsert PR) and SpiderMonkey. I don't have permissions to commit this, could you please have another look and if looks ok, commit it? |
|
Sure thing. I didn't review these for completeness, just gave them a quick glance and executed them. I found it puzzling that you didn't have permission to land this but then I realized it's because of the change to |
|
No problem, thank you for the help :) |
These tests are currently being executed in SpiderMonkey as
non262tests, and were exported using thetest262-export.pyscript. Linting errors were then corrected.