🐛 Fix instances where allowConsoleError swallows return values

[rsimha]

#14592 (comment) raised the issue that allowConsoleError(func) as written today will swallow the return value of func if it were to return something, like a Promise.

This PR does three two things:

1. Returns the result of func at the end of allowConsoleError.
2. Fixes all instances of tests that fail the above check.
3. Adds a presubmit check to prevent tests from failing to return the value of func from allowConsoleError in cases where func does return something.
   This will be added as a new .eslint plugin in a separate PR. See Write an eslint plugin to prevent allowConsoleError from swallowing return values #15242.

Related to #14592 (comment)

[rsimha]

/to @choumx @danielrozenberg

[jridgewell]

Lookbehinds are brand new, impossible to polyfill.

[rsimha]

Why is this a problem? This is just a presubmit check running on Travis.

[jridgewell]

Only available on Node 9, and needs a flag on Node 8. http://node.green/#ES2018-features--RegExp-Lookbehind-Assertions

[jridgewell]

\\(\\)=> \\{[\\n\\r\\s]*return fails to capture expression bodies, non-arrows, parameters, any anything that's not immediately returning.

[rsimha]

That's true. It's a simple check for the kind of usage we currently have. What do you suggest this be changed to?

[jridgewell]

An eslint plugin.

[rsimha]

Okay, that's out of scope of this PR, so I'll revert this presubmit check and cough let you write a new plugin since you're more familiar with this than I am :)

[jridgewell]

/**
 * Copyright 2018 The AMP HTML Authors. All Rights Reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS-IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
'use strict';

module.exports = {
  meta: {
    fixable: 'code',
  },

  create(context) {
    return {
      CallExpression(node) {
        //if (!/test-/.test(context.getFilename())) {
        //  return;
        //}
        debugger
        const {callee} = node;
        if (callee.type !== 'Identifier' ||
            callee.name !== 'allowConsoleError') {
          return;
        }
        
        const {parent} = node;
        if (!parent || parent.type === 'ReturnStatement') {
          return;
        }
        
        const sourceCode = context.getSourceCode();
        const fn = sourceCode.getText(node);
        if (!fn.includes('return')) {
          return;
        }
        
        context.report({
          node,
          message: 'Must return allowConsoleError if callback contains a return',
          fix(fixer) {
            return fixer.replaceText(node, `return ${fn}`);
          },
        });
      }
    };
  },
};

[rsimha]

That won't work in cases like this:

amphtml/test/functional/utils/test-bytes.js

Lines 50 to 52 in 18520b4

	allowConsoleError(() => { expect(() => {
	return stringToBytes('ab☺');
	}).to.throw(); });

I've logged #15242 to track the addition of an eslint rule.