Unmute tests for amp-experiment variant replacement.

[lannka]

The root cause of those tests failures are actually the use of rethrowAsync: https://github.com/ampproject/amphtml/blob/master/src/service/url-expander/expander.js#L327

We might consider remove all the usage of rethrowAsync in code base. @rsimha what you think?

This PR is to unblock #20221

also part of #16916

[rsimha]

From a testing point of view, removing rethrowAsync will greatly reduce the amount of flakiness we see. However, is there a valid use case for doing it in the runtime? Why was it added in the first place? Does it prevent the runtime from blocking when it encounters an error, perhaps?

@jridgewell @erwinmombay @choumx

[dvoytenko]

Should this be happening here? Or should we replace rethrowAsync upstream with the user().log()? E.g. the logic that says "ignore errors and return empty string" is already part of this system. This duplicates it slightly.

[lannka]

agree this is a duplicates. will need a root fix. was plan to have this as a temporary work around to unblock your PR

[dvoytenko]

Up to you - you could allow my PR w/o unmuting these tests :)

[lannka]

will let your PR go if you can confirm the tests pass locally

[dvoytenko]

Yep. Can confirm that it works with patches.

[dvoytenko]

@rsimha Not sure if this is the best forum for rethrowAsync discussion, but to kick it off. The original intent with rethrowAsync was this: "Don't block the code, but still treat it as an error downstream, including reporting to console, analytics services, diagnostics logging". I realize that it caused us some paint with tests, but I think we could try to address that pain in a couple of different ways and maybe that'd be an improvement over the current state. E.g. something like:

function rethrowAsync(error) {
  if (TESTING) {
    // E.g. these errors could be then used for test assertions.
    TESTING.collectErrors(error);
  } else {
    // Normal PROD behavior.
    setInterval(() => {throw error;});
  }
}

With something like this, the if (TESTING) part would be stripped from PROD binary.

Overall, IMHO, I prefer rethrowAsync to user().log() and dev().log(). I'd prefer we relied on logging packages as little as possible. rethrowAsync is essentially a proxy to throw new Error() and reads much cleaner. But that's a bigger conversation.

[rsimha]

That was really useful, @dvoytenko! Agree, let's kick off a separate discussion.

[lannka]

let's discuss the need of rethrowAsync here #20252

[mrjoro]

From the discussions in #20252 it sounds like @torch2424 will be working on this now? Will that be in this PR or will that be a different one?

[lannka]

@mrjoro this PR is not needed. closing