search

ibm-js / ecma402

ECMA-402 JavaScript Internationalization API "shim"
Other
34 stars 21 forks source link

CanonicalizeLocaleList() update. #48

Closed JCEmmons closed 10 years ago

JCEmmons commented 10 years ago

I went back to the ECMA-402 and took another look at this method, and found out that I was following the spec exactly. I think this should fix the problem with tests not running properly on Safari/MAC, but I won't know for sure until I see the results from TravisCI, since I'm having some trouble getting test:remote to work properly with Saucelabs. Fixes #35 maybe... Most of the code changes here are line for line out of the spec, so please don't shoot the messenger.

cjolif commented 10 years ago

As discussed offline unfortunately the PRs are not going through TravisCI. You could enable it on your fork though. That said I will make sure today to test the PR on MacOS and let you know the result.

cjolif commented 10 years ago

So after running the tests locally on MacOS/Safari I confirm this PR is fixing the currently reported issue that is #35. However there is still an issue with test 12_2_2_b which is kind of blocking the test suite (this blocks on this test and does not go further but don't report error either). I suspect this time the issue is more with the test suite than with the library code but I will try to double check.

cjolif commented 10 years ago

The "new" problem comes from https://github.com/ibm-js/ecma402/blob/master/tests/intl402/suite/ch12/12.2.js#L53 and https://github.com/ibm-js/ecma402/blob/master/tests/intl402/harness/testIntl.js#L103

The test suite is trying to redefine the "1" property on Object prototype. Presumably to check that argument index 1 is not accessed in the ctor function as a single argument is passed (just below when instanciating DateTimeFormat).

Apparently Safari does not appreciate that redefinition on Object.prototype.

In addition to the fact this is causing trouble on Safari I suspect this code won't necessarily produce the expected behavior in other browsers. Indeed, if I'm not mistaken arguments "array" from the function is actually not based on Object.prototype? So I'm not sure what it can catch?

Or maybe the purpose of this code is different than what I suppose?

In any case I would propose to comment this test to make the full suite run on all browsers and discuss with the test suite owner about it so that we can better understand what he is trying to achieve and if that is indeed correct.

JCEmmons commented 10 years ago

I've had a few discussions with Norbert about the purpose of such tests. The one thing we have to remember here is that the test suite was originally designed for people to test native browser implementations of Intl, not necessarily javascript implementations such as ours that are sitting on top of the browser. As such, tests like the one that is failing are in the suite, in order to try to insure that people can't do malicious things by messing around with Object.prototype.

I think that for the time being, chasing things like this in the test suite are of limited value to us ( as indeed there are a few other similar tests that I have already commented out ). So I'm OK to remove 12.2.2.b until we have time to investigate it further. We've got more important things to work on.