-
Notifications
You must be signed in to change notification settings - Fork 452
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
Tests for proposed Annex B semantics #969
Conversation
harness/propertyHelper.js
Outdated
return !Object.prototype.hasOwnProperty.call(obj, name); | ||
var result = !Object.prototype.hasOwnProperty.call(obj, name); | ||
// Restore property (no-op if not deleted) | ||
Object.defineProperty(obj, name, descriptor); |
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.
We've already discussed this before in other issues.
I don't think we should re-add the property. Once it's used, it should be consumed in the sense of atomic unit tests. To test anything in top of that, another set of object/properties would be recommended instead.
We can't avoid the side effect a configurable property will be deleted. This operation is not only restoring it, but triggering a [[DefineOwnProperty]]
operation and this might cause other side effects in other test files.
If I'm not wrong, @bterlson was in those other discussions about this.
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.
OK, I guess that could come up if you are using a Proxy that there are other side effects, but for ordinary objects, there should be no side effect to deleting a property and re-adding it the same way.
Anyway, sure, we can leave a side effect in this one, but in that case, could we put something else in propertyHelper.js to test for a non-configurable property and not mutate the object? This would be useful to allow for the multiple observations of the same property on the same object (as in the tests in the following patch). Note that we can still not be sure that it won't be side-effecting in the case of Proxies, however.
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.
while it might not leave side effects beyond Proxies, it's beyond the responsibility for verifyConfigurable restore a state, as an extra step. Tests should follow the minimum possible steps to confirm an assertion. The best of all options would be removing the delete operation, but that's necessary as we already found parts where the descriptor was not matching the actual configurability of a property.
If the property is sent to be tested it should be consumed and that's it.
There's also the problem with non extensible objects:
Object.preventExtensions(Object);
delete Object.length;
Object.defineProperty(Object, 'length', { value: 42 }); // will throw
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.
Oh right, you can delete properties from non-extensible objects. Sorry for my confusion.
Well, would you object to adding a function which just does the test and doesn't do the delete? I'd like to use a factored-out verifyConfiguable, and adding the property back in my test feels like a layering violation (who would think that a function with that name would delete a property? It feels like depending on an implementation detail).
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.
Well, would you object to adding a function which just does the test and doesn't do the delete?
I agree with you this would be the proper approach. I really want to have some that only checks for Object.getOwnPropertyDescriptor(obj, 'prop').configurable;
, but that's when we got the history of finding false positives from runtimes.
I have plans to create an easier to consume harness api for properties, where I would verify it like:
verifyProperty(obj, 'prop', {
configurable: false,
enumerable: true,
writable: false
});
So the tests won't need to worry about the order. e.g. verifyWritable
being called after verifyConfigurable
.
Using this same function, we can extend it with options, let's say:
verifyProperty(obj, 'prop', {
configurable: false,
enumerable: true,
writable: false
}, {
descriptorOnly: true // avoid changing the actual values or deleting properties
});
or even:
verifyProperty(obj, 'prop', {
configurable: false,
enumerable: true,
writable: false,
__descriptorOnly: true
});
The function signature is only a bikeshed, but a new PR for it might be a good starting point to discuss the options.
Does it work for you?
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.
No, because in this test I check property attributes of the same property multiple times from different places.
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.
I changed this to have a restore
option instead, which is doing exactly what you're doing here, but now in the new API: #979
configurable: false | ||
}); | ||
|
||
$.evalScript(` |
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.
$262.evalScript
, but I wonder if we can't just use eval
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.
Oh oops, will fix.
We need to be eval'ing another script; this has different behavior. I'm getting at the global case in this test, not the eval behavior. test262 is actually missing coverage of other properties of Annex B 3.3 extensions to GlobalDeclarationInstantiation.
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 for the clarification.
the indentation fixes from a621155 found a small git conflict - whitespace only. |
I was trying to find side effects from the new verifyConfigurable api and found this file only failing with the new approach.
|
verifyEnumerable(fnGlobalObject(), 'f'); | ||
verifyWritable(fnGlobalObject(), 'f'); | ||
verifyNotConfigurable(fnGlobalObject(), 'f'); | ||
`) |
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.
Nit: ;
verifyEnumerable(fnGlobalObject(), 'f'); | ||
verifyWritable(fnGlobalObject(), 'f'); | ||
verifyNotConfigurable(fnGlobalObject(), 'f'); | ||
`) |
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.
Nit: ;
verifyNotEnumerable(fnGlobalObject(), 'f'); | ||
verifyWritable(fnGlobalObject(), 'f'); | ||
verifyConfigurable(fnGlobalObject(), 'f'); | ||
`) |
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.
;)
verifyNotEnumerable(global, "f");\ | ||
verifyWritable(global, "f");\ | ||
verifyConfigurable(global, "f");{ function f() { } }' | ||
); |
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.
For new multi-line strings, I recommend that we stick to template strings (I also recognize that this came from existing code). Same for all occurences
@littledan do you have any plans to move the normative change further on ecma262? I can help with the fixes here when it's time for an update. |
Sorry for dropping the ball on the review feedback here. I believe this patch has consensus and is just waiting on fixing up the code review feedback to land. |
no problem, I might be able to work on this tomorrow. I'll ping you back then. |
Thanks for your help! |
These tests are againt a proposed fix for tc39/ecma262#753 They seem to pass in V8, JSC and SpiderMonkey, though ChakraCore hews slightly closer to the previous specification.
5989fe1
to
88a673b
Compare
Updated tests to address all of the code review comments (except @rwaldron 's suggestion to change from ' to ` in orthogonal template files, which doesn't need to be part of this patch). PTAL. |
@littledan @leobalter sorry for letting this sit for so long. I've followed the chain of issues and PRs, all the back to the notes and verified that this does have consensus, so I will get this landed today. |
@littledan can you help me pointing to the consensus part? I felt a bit lot trying to find it, maybe in the notes docs? |
I found the link to the consensus but it's so weird the notes are not well captured...
|
No description provided.