Bug - 634379 sandbox tests #109
Conversation
|
Irakli: can you address the last few review comments? It'd be nice to get this landed! |
|
Myk I don't think you have reviewed this change yet. Comments you sew before were for the change this one depended on, which landed quite a while ago. I merged pull req branch with master, and now you can see only actual changes (not a changes for dependencies any more). |
| }; | ||
|
|
||
| exports.Assert = require("./asserts").create(Object); | ||
| exports["test inheritence"] = function (assert) { |
There was a problem hiding this comment.
Nit: inheritence -> inheritance
There was a problem hiding this comment.
Nit: it'd make sense to separate these two lines with a line of whitespace, since they aren't particularly connected.
| * 1. Property descriptor has `configurable` attribute with value `false`. | ||
| * 2. [[Delete]] throws a `TypeError` in strict mode. | ||
| * 3. [[Delete]] does not succeeds. | ||
| * 4. Defining same named property throws `TypeError`. |
There was a problem hiding this comment.
Nit: succeeds -> succeed
Nit: same named -> same-named
|
@mykmelez I'd suggest closing this without landing, since behavior of ES5 additions in cross compartments is different across versions FF5, Aurora, Nightly and it would be a lot of work trying to keep this tests passing across all of this versions. In addition we're in process of exploring jsm and function wrapper based modules. Please close this pull request and bug with won't fix if you agree or let me know if you think otherwise. |
|
My only concern about doing so is that it means we don't have insight into the ES5 features we can count on, since we have to keep our sandbox-based modules working on Firefox 4 and up (until we increment the minimum version of Firefox we support or stop using sandboxes). How about marking tests that are expected to fail because of platform bugs? Over in pull request 198, Shane Tomlinson is adding an expectFail method that can formally wrap such tests, so they don't cause test runs to fail but do show up in the test run logs. |
|
This pull request is still work in progress so please hold on with a next round of review! |
|
Now it's ready for another round! |
|
Rereviewed! Mostly just nits, but with a few substantive issues. See the comments above for details. |
No description provided.