Skip to content
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

Merged
merged 2 commits into from Oct 6, 2017

Conversation

dilijev
Copy link
Contributor

@dilijev dilijev commented Apr 17, 2017

No description provided.

// This code is governed by the BSD license found in the LICENSE file.

/*---
es5id: 6.2.3
Copy link
Member

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 :)

};

Object.getOwnPropertyNames(canonicalizedTags).forEach(function (tag) {
let locale = Intl.getCanonicalLocales(tag);
Copy link
Member

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.

Copy link
Contributor

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).

let locale = Intl.getCanonicalLocales(tag);
if (!canonicalizedTags[tag].includes(locale[0])) {
$ERROR("For " + tag + " got " + locale + "; expected one of " +
canonicalizedTags[tag].join(", ") + ".");
Copy link
Member

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(", ")}`);

"x-foo": ["x-foo"]
};

Object.getOwnPropertyNames(canonicalizedTags).forEach(function (tag) {
Copy link
Member

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(", ")}`);
});

"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
Copy link
Member

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

/*---
es5id: 6.2.3
description: Tests that language tags are canonicalized in return values.
author: Norbert Lindenberg
Copy link
Member

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 :)

es5id: 6.2.3
description: Tests that language tags are canonicalized in return values.
author: Norbert Lindenberg
includes: [testIntl.js]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not necessary.

@dilijev
Copy link
Contributor Author

dilijev commented Apr 18, 2017

@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 Intl.getCanonicalLocales.

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 test/intl402/Intl/getCanonicalLocales/ directory?

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?

@leobalter
Copy link
Member

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 test/intl402/Intl/getCanonicalLocales/ directory?

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.

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?

If you can afford this time, I'm +1 to it and can help reviewing!

@leobalter
Copy link
Member

I'm looking forward to review the new update. Please ping me when you push it.

@rwaldron
Copy link
Contributor

@dilijev nudge ;)

@dilijev
Copy link
Contributor Author

dilijev commented Jul 5, 2017

@rwaldron Sorry, I've had a bunch of other priorities. This is on my list for about 3 weeks from now :)

@rwaldron
Copy link
Contributor

@dilijev another friendly nudge :D

@dilijev
Copy link
Contributor Author

dilijev commented Aug 11, 2017

@rwaldron Yep, I know I'm past my promised date. Super swamped! This is still on my radar!

@rwaldron
Copy link
Contributor

rwaldron commented Oct 3, 2017

@leobalter or @spectranaut can we do the fix up work for @dilijev?

@dilijev
Copy link
Contributor Author

dilijev commented Oct 4, 2017

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.

@leobalter
Copy link
Member

I'll be glad to take care of this, but bear with me, this will be during my free time this week.

@leobalter leobalter self-assigned this Oct 4, 2017
@spectranaut
Copy link
Contributor

I have time to take this now :)

@spectranaut
Copy link
Contributor

@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.

@spectranaut
Copy link
Contributor

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 canonicalizedTags to an include, because its being use in two different tests in two different directories -- to be honest, there are so many tests and so much complexity in Test262, that rather than focusing on the DRYness of the code we instead focus on how easy it is to read any given test file, specifically with the goal of understanding what went wrong given a failure. Given this context, leaving canonicalizedTags in two separate files in two separate directories is ok, possibly even preferred.

@rick
Copy link

rick commented Oct 5, 2017

@rick rick mentioned this pull request Oct 5, 2017
@leobalter
Copy link
Member

@rwaldron why you can't simply hold an easier @ handler?

@spectranaut (...) we instead focus on how easy it is to read any given test file, specifically with the goal of understanding what went wrong given a failure.

+1000

@leobalter leobalter merged commit 1afb7c7 into tc39:master Oct 6, 2017
@dilijev
Copy link
Contributor Author

dilijev commented Oct 10, 2017

@spectranaut (...) we instead focus on how easy it is to read any given test file, specifically with the goal of understanding what went wrong given a failure.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants