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 version of intl402/6.2.3.js which tests via Intl.getCanonicalLocales #986
Conversation
test/intl402/6.2.3_b.js
Outdated
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
es5id: 6.2.3 |
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.
this should use the esid
tag instead. It's clearly not part of the es5 specs :)
test/intl402/6.2.3_b.js
Outdated
}; | ||
|
||
Object.getOwnPropertyNames(canonicalizedTags).forEach(function (tag) { | ||
let locale = Intl.getCanonicalLocales(tag); |
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 this test case seems legit, the place where this file is named and placed needs improvement.
test/intl402/6.2.3.js
is using a legacy format where all the tests files were named after specs chapter references. That proved hard to maintain and consume as well. Unfortunately, renaming all the legacy file names is at a hard cost I cannot afford for now with time, but we're slowly reaching progress.
this file can be placed in the test/intl402/Intl/getCanonicalLocales/
folder, the name should reflect what is checked here. The other files in the same folder should give a hint and what is done.
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.
The esid
should also be: sec-intl.getcanonicallocales
and not sec-canonicalizelanguagetag
. We're not in the business of targeting tests for specific abstract operations. Those operations may change, but the exposed and observable APIs should not (unless they do, but that's a different discussion).
test/intl402/6.2.3_b.js
Outdated
let locale = Intl.getCanonicalLocales(tag); | ||
if (!canonicalizedTags[tag].includes(locale[0])) { | ||
$ERROR("For " + tag + " got " + locale + "; expected one of " + | ||
canonicalizedTags[tag].join(", ") + "."); |
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.
Using $ERROR
directly is another legacy issue we're avoiding. We should use the internal assert api instead:
assert(canonicalizedTags[tag].includes(locale[0]), `For ${tag} got ${locale}; expected one of ${canonicalizedTags[tag].join(", ")}`);
test/intl402/6.2.3_b.js
Outdated
"x-foo": ["x-foo"] | ||
}; | ||
|
||
Object.getOwnPropertyNames(canonicalizedTags).forEach(function (tag) { |
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.
Object.keys
would do the work here. If you're also consuming the values, Object.entries
might help avoiding using canonicalizedTags[tag]
.
Object.entries(canonicalizedTags).forEach({ tag, expected } => {
assert(expected.includes(locale[0]), `For ${tag} got ${locale}; expected one of ${expected.join(", ")}`);
});
test/intl402/6.2.3_b.js
Outdated
"cmn-hans-cn": ["cmn-Hans-CN", "cmn-Hans", "cmn"], | ||
"es-419": ["es-419", "es"], | ||
"es-419-u-nu-latn": ["es-419-u-nu-latn", "es-419", "es", "es-u-nu-latn"], | ||
// -u-ca is incomplete, so it will not show up in resolvedOptions().locale |
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 should remove this line
test/intl402/6.2.3_b.js
Outdated
/*--- | ||
es5id: 6.2.3 | ||
description: Tests that language tags are canonicalized in return values. | ||
author: Norbert Lindenberg |
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.
Norbert Lindenbern might hold the copyrights but is not the author of this test. You are :)
test/intl402/6.2.3_b.js
Outdated
es5id: 6.2.3 | ||
description: Tests that language tags are canonicalized in return values. | ||
author: Norbert Lindenberg | ||
includes: [testIntl.js] |
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.
this is not necessary.
@leobalter Thanks for the review! I'll make some changes but I'd like some advice. I took the test payload from the pre-existing 6.2.3 test and wrote a new test to exercise the payload with Reasoning for this file as it is (to some extent): I looked at the existing file and felt the array of test cases contained there were a good set of cases for Intl.getCanonicalLocales. I didn't necessarily want to add to that test because I wasn't sure how Copyright would be handled, and since it's a direct fork of that test payload which exercises a different feature. Any advice for determining copyrights in a test forked in this way? Should I add these tests to the existing test and update the style (and move the whole test to the Splitting tests which exercise this payload between two different directories seems like it would be annoying to maintain. Also perhaps I should refactor the test payload (the object full of test cases) to a separate file and include that in both tests? |
for now yes. We have plans to not require the copyrights header per file as the root license in this project should be enough. That's happening in a near future, but for now this is the way to roll.
If you can afford this time, I'm +1 to it and can help reviewing! |
I'm looking forward to review the new update. Please ping me when you push it. |
@dilijev nudge ;) |
@rwaldron Sorry, I've had a bunch of other priorities. This is on my list for about 3 weeks from now :) |
@dilijev another friendly nudge :D |
@rwaldron Yep, I know I'm past my promised date. Super swamped! This is still on my radar! |
@leobalter or @spectranaut can we do the fix up work for @dilijev? |
Yep, I have the box checked to allow edits from maintainers so you should be able to push to my branch. Sorry about the delay. I'll let you know if I start on the fix up work first so we don't duplicate effort. |
I'll be glad to take care of this, but bear with me, this will be during my free time this week. |
I have time to take this now :) |
@leobalter or @rwaldron -- it seems like the original file (renamed to 6.3.2_a.js) is a test of https://tc39.github.io/ecma402/#sec-internal-slots -- Can you verify my intuition or correct my intuition? I might move this file to the appropriate directory, as discussed above. |
A few fix ups, @dilijev, thanks for your initial commit! @rick or @leo can review for merging. About the refactoring you mentioned -- it would be nice to put 6.3.2_a.js in the correct directory, but I'm leaving it were it is for now rather than digging into where it should go. If you feel like trying to suss that out, then great :) About moving |
@rwaldron why you can't simply hold an easier @ handler?
+1000 |
SGTM Thanks @spectranaut and @leobalter for your help landing this and your patience. Turns out we've decided not to do the bit of work that stalled me out on this change :P |
No description provided.