💅 for readability, it might make sense to assign this expression to a variable to indicate what and why this is happening.

const canRenderAsLegacyMetric = /* ...expression... */;

// Do this to disturb existing templates as little as possible
if (canRenderAsLegacyMetric) {
  return /* ... */;
}

Can you explain what you mean by the term "legacy" here? This doesn't quite make sense to me, since the accountId is only set if this is true, which would imply that a cross-account alarm only works if the metric is in a "legacy" format of some kind. But that doesn't make sense, since cross-account alarms are a new feature.

The "legacy" term refers to the fact that cloudwatch has two ways to provision metrics on alarms, and only one way is being updated with new features like cross-account alarms. We want to avoid causing unnecessary diffs, so we shepherd metrics that do not require these "new" features to the "legacy" properties - the new way to provision metrics is via the CfnAlarm.MetricDataQueryProperty.

Here accountId is only set if canRenderAsLegacyMetric is false, not true. So it is correctly named here. (what seems to be the confusion is that github is hiding the unchanged lines and makes it seem like accountId is inside the if (canRenderAsLegacyMetric) scope when it is actually not.)

aha, thank you for the explanation! The github diff was confusing me here as well :)

Can you add a test that uses the method of setting the account property on the metric instead of using attachTo? There should be a unit test for each way that it is possible to use the feature.

I added an additional test that uses the account property, but I do want to push back on this a bit (or ask for clarification). Under the hood, metric.AttachTo() simply updates the metric object with the account property, so I see it as a convenience method and not an additional way to use this feature. To me, it seems like the second test duplicates the first. Does this still warrant an additional unit test?

I suppose it is somewhat of a grey area. But I still think the extra test is helpful. Your reasoning for the additional unit test being unnecessary is based on an implementation detail. What if that changes? In this particular scenario, it doesn't seem likely. But in general, unit tests should test the API in all ways that a customer could use it, and make sure that the correct behaviour/result happens in all of them. So that if someone accidentally breaks something by changing an implementation detail, then the unit tests will catch it.

aha, thank you for the explanation! The github diff was confusing me here as well :)

I suppose it is somewhat of a grey area. But I still think the extra test is helpful. Your reasoning for the additional unit test being unnecessary is based on an implementation detail. What if that changes? In this particular scenario, it doesn't seem likely. But in general, unit tests should test the API in all ways that a customer could use it, and make sure that the correct behaviour/result happens in all of them. So that if someone accidentally breaks something by changing an implementation detail, then the unit tests will catch it.