Refactor dayPeriod formatting tests to be robust to CLDR changes

[ptomato]

Taken from Richard's suggestion in #4428 (review)

[gibson042]

LGTM modulo variable declarations.

[gibson042]

This is by no means blocking, but I wonder if a harness file should define some formatToParts assertion helpers:

function assertFormatPart(part, type, value, label) {
  var suffix = label ? ', ' + label : '';
  assert.sameValue(part.type, type, 'part type' + suffix);
  if (value === undefined) return;
  assert.sameValue(part.value, value, 'part value' + suffix);
}

function assertFormatParts(parts, expectations, label) {
  var suffix = label ? ', ' + label : '';
  assert.sameValue(parts.length, expectations.length, 'parts count' + suffix);
  for (var i = 0; i < parts.length; i++) {
    assertFormatPart(parts[i], expectations[i][0], expectations[i][1],
      suffix && (suffix + ' part ' + i));
  }
}

for use in files like this:

for (var h = 0; h < 24; h++) {
  var parts = long.formatToParts(inputs[h]);
  assertFormatParts(parts, [['dayPeriod']], 'hour ' + h + ' long dayPeriod');
  …
  var partsNumeric = longNumeric.formatToParts(inputs[h]);
  assertFormatParts(partsNumeric,
    [['hour', String((h % 12) || 12)], ['literal'], ['dayPeriod']],
    'numeric hour ' + h + ' must precede long dayPeriod');

  counts[i]++;
  prevDayPeriod = dayPeriod;
}

[ptomato]

This seems interesting. I'd wait to see what kinds of format parts assertions we need once we get the tests into a better state. Want to open an issue with this suggestion?

[gibson042]

#4451

[anba]

Two issues:

1. The updated tests are now too loose. For example it allows 1 noon, 2 in the afternoon, 3 in the evening, 4 at night, and the rest in the morning.
2. The transition checks seem more complicated than necessary.

How about something like this instead?

// Definitions for inputs, expectedDayPeriods, and formatters elided.

const dayPeriods = inputs.map(date => long.format(date));

// Ensure all day periods are valid.
assert(dayPeriods.every(d => expectedDayPeriods.includes(d)));

// Known day periods for specific hours.
assert.sameValue(dayPeriods[0], "at night");
assert.sameValue(dayPeriods[6], "in the morning");
assert.sameValue(dayPeriods[12], "noon");
assert.sameValue(dayPeriods[15], "in the afternoon");
assert.sameValue(dayPeriods[18], "in the evening");
assert.sameValue(dayPeriods[22], "at night");

// Noon happens exactly once per day.
assert.sameValue(dayPeriods.filter(d => d === "noon").length, 1);

// Ensure all transitions are valid.
for (let h = 1; h < dayPeriods.length; h++) {
  if (dayPeriods[h] !== dayPeriods[h - 1]) {
    let currentIndex = expectedDayPeriods.indexOf(dayPeriods[h]);
    let previousIndex = expectedDayPeriods.indexOf(dayPeriods[h - 1]);

    // The previous-index plus one matches the current index, possibly by wrapping around.
    assert.sameValue(
      currentIndex,
      (previousIndex + 1) % expectedDayPeriods.length
    );
  }
}

for (let h = 0; h < inputs.length; h++) {
  assert.sameValue(
    longNumeric.format(inputs[h]),
    // Hour "00" is represented as "12".
    ((h % 12) || 12) + " " + dayPeriods[h],
    "numeric hour must precede dayPeriod"
  );
}

[gibson042]

The updated tests are now too loose. For example it allows 1 noon, 2 in the afternoon, 3 in the evening, 4 at night, and the rest in the morning.

I think I would consider that acceptable. It's not our place to define the association of hours and day periods; that is the purview of upstream projects like CLDR.

// Known day periods for specific hours.
assert.sameValue(dayPeriods[0], "at night");
assert.sameValue(dayPeriods[6], "in the morning");
assert.sameValue(dayPeriods[12], "noon");
assert.sameValue(dayPeriods[15], "in the afternoon");
assert.sameValue(dayPeriods[18], "in the evening");
assert.sameValue(dayPeriods[22], "at night");

I'd rather not include these kinds of assertions, but I'm not strongly opposed. However, I definitely wouldn't bake in an assumption that hour 0 is necessarily "at night".

The transition checks seem more complicated than necessary.

I think it's a worthwhile tradeoff for reducing the maintenance burden for reacting to upstream changes (e.g., introduction of "midnight", expansion of "noon" to more than one hour, a new term that splits "in the morning"). Basically, I want to not touch these files at all in response to likely CLDR changes, and touch only one line each even for surprising ones.

But simplifying the checks while upholding those goals seems good. I like the idea of asserting that all day periods are represented and then iterating over those rather than over hours.

[ptomato]

Yeah, agreed with Richard — the general direction we have been discussing is that tests for locale-defined results become looser. Otherwise we are just testing the specifics of the underlying version of ICU. Currently test262 is only runnable for engines with a specific version of ICU (and if you are using any other i18n library, forget it) and that's the situation I'm trying to chip away at here 😄

I'll try to simplify the checks though.

[anba]

Removing too many checks makes these tests rather useless, I'm almost more in favour of just removing them altogether instead of allowing nonsensical results. The latest set of changes made this even worse, it's now allowed for implementations to return:

0 -> 0 noon
1 -> 1 in the afternoon
2 -> 2 in the evening
3 -> 3 at night
4 -> 4 in the morning
5 -> 5 noon
6 -> 6 in the afternoon
7 -> 7 in the evening
8 -> 8 at night
9 -> 9 in the morning
10 -> 10 noon
...

[gibson042]

Updated suggestion incorporating @anba's approach:

// Each expected dayPeriod value must be contiguous, and
// b) represented in sequence.
var expectedDayPeriods = [
  'in the morning',
  'noon',
  'in the afternoon',
  'in the evening',
  'at night'
];

// Cover all 24 hours of a single day.
var inputs = [];
for (var h = 0; h < 24; h++) {
  inputs.push(new Date(2017, 11, 12,  h, 0, 0, 0));
}

// Verify complete and exclusive representation.
var formatter = new Intl.DateTimeFormat('en', {
  dayPeriod: 'long'
});
var observedDayPeriods = [];
var unexpectedDayPeriods = [];
for (var h = 0; h < 24; h++) {
  var dayPeriod = formatter.format(inputs[h]);
  observedDayPeriods.push(dayPeriod);
  if (expectedDayPeriods.indexOf(dayPeriod) === -1) {
    unexpectedDayPeriods.push(dayPeriod);
  }
}
var unusedDayPeriods = expectedDayPeriods.filter(function (dayPeriod) {
  return observedDayPeriods.indexOf(dayPeriod) === -1;
});
assert.compareArray(unexpectedDayPeriods, [],
  'unexpected dayPeriods: ' + unexpectedDayPeriods.join());
assert.compareArray(unusedDayPeriods, [],
  'unused dayPeriods: ' + unusedDayPeriods.join());

// Verify ordering, accounting for the possibility of one value spanning day transitions.
var transitionCount = 0;
for (var h = 0; h < 24; h++) {
  var dayPeriod = observedDayPeriods[h];
  var prevDayPeriod = observedDayPeriods.at(h - 1);
  if (dayPeriod === prevDayPeriod) continue;
  transitionCount++;
  var i = expectedDayPeriods.indexOf(dayPeriod);
  assert.sameValue(prevDayPeriod, expectedDayPeriods.at(i - 1),
    dayPeriod + ' must be preceded by ' + prevDayPeriod);
}
assert.sameValue(transitionCount, expectedDayPeriods.length,
  'dayPeriods must be contiguous');

var numericFormatter = new Intl.DateTimeFormat('en', {
  dayPeriod: 'long',
  hour: 'numeric'
});
for (var h = 0; h < 24; h++) {
  assert.sameValue(
    numericFormatter.format(inputs[h]),
    // Hour "00" is represented as "12".
    ((h % 12) || 12) + ' ' + observedDayPeriods[h],
    'numeric hour must precede dayPeriod'
  );
}

[ptomato]

Thanks, I've adopted that suggestion (but with a workaround instead of using Array.prototype.at) and updated the PR.

[gibson042]

Suggestion: indicate that this is obviously a workaround.

Suggested change

	function relativeIndex(arr, relIndex) {
	function arrayAt(arr, relativeIndex) {

Or alternatively, implement it manually at the site of use:

-  var prevDayPeriod = relativeIndex(observedDayPeriods, h - 1);
+  var prevDayPeriod = observedDayPeriods[(h || observedDayPeriods.length) - 1];
   if (dayPeriod === prevDayPeriod) continue;
   transitionCount++;
   var i = expectedDayPeriods.indexOf(dayPeriod);
-  assert.sameValue(prevDayPeriod, relativeIndex(expectedDayPeriods, i - 1),
+  assert.sameValue(prevDayPeriod, expectedDayPeriods[(i || expectedDayPeriods.length) - 1],
     dayPeriod + ' must be preceded by ' + prevDayPeriod);

[gibson042]

#4451