Wrap a single test in test-amp-form.js that expect a console error with allowConsoleError

[danielrozenberg]

Followup to #14432

This only fixes one test which was previously assigned to me, because each and every test in this file is weird in its own way and I don't want to go down this rabbit hole

[rsimha]

@choumx knows these tests better than I do.

[danielrozenberg]

/to @rsimha I rebased it with the changes introduced in #14590 - without this PR, the test will raise a warning about not using allowConsoleError. With this PR, it raises a warning about using allowConsoleError without using console.error

[rsimha]

As discussed offline, it's possible your test is doing something asynchronously, that's causing the console error to appear after the anonymous function passed to allowConsoleError has returned. You'll need to make the part(s) of your test that expect the error synchronous before wrapping them in allowConsoleError.

Examples:

This will warn...

console.error('foo');

... as will this...

allowConsoleError(() => {
  noError();
});

... but this will work...

noError();

... and so will this...

allowConsoleError(() => {
  console.error('foo');
});

[dreamofabear]

Nit: getAmpFormSync.

[dreamofabear]

This has the same problem I mentioned in chat. The returned promise won't be chained (unless this has been since fixed).

[rsimha]

Noted. Not yet fixed.

[danielrozenberg]

I'm not sure what you're referring to

[dreamofabear]

Place a breakpoint on line 577. I believe it won't get triggered.

This is because sinon chains returned promises, but allowConsoleError() currently swallows it. What we need is for allowConsoleError(f) to detect promises returned in by f and chain them. Then, we'll do:

return allowConsoleError(() => { 
  ...
});

[rsimha]

Fixed in #15228. With this, you can return allowConsoleError. Improperly written tests will fail presubmit checks.

[aghassemi]

Thanks for the Pull Request. Since this PR has been stale for a while, I am closing it. Please feel free to reopen as needed.