Ban using "toBeTruthy()" to match Promises, which is error-prone. Promises are always truthy - it's likely that instead the developer intended to match the result of the Promise.
PiperOrigin-RevId: 184560412
diff --git a/docs/ban-expect-truthy-promise.md b/docs/ban-expect-truthy-promise.md
new file mode 100644
index 0000000..47ba723
--- /dev/null
+++ b/docs/ban-expect-truthy-promise.md
@@ -0,0 +1,33 @@
+<!-- FIXME(alexeagle): generate the docs from the sources -->
+
+## Don't expect promises toBeTruthy()
+
+Don't write expectations like this:
+
+ expect(returnsPromise()).toBeTruthy();
+
+Promises are always truthy, so this assertion will never fail. Usually, the
+intention was to match the result of the promise. If that's the case, simply
+add an `await`.
+
+ expect(await returnsPromise()).toBeTruthy();
+
+### In Protractor tests
+
+If you're not writing a Protractor test, you can safely ignore this section.
+
+In the past, Protractor tests have patched `expect()` to automatically unwrap
+promises, which made these assertions work as expected without needing an
+`await`. However, the [control flow is deprecated][1] and will be removed in
+Selenium WebDriver 4.0, and these assertions are now a bug. You'll
+need to `await` the promise as above.
+
+If you can't `await` the promise because your tests need the control flow, you
+can use a more specific matcher.
+
+ expect(returnsPromise()).toBe(true);
+
+This assertion will work as expected for now and will fail when the control flow
+is finally removed.
+
+[1]: https://github.com/SeleniumHQ/selenium/issues/2969
diff --git a/internal/tsc_wrapped/package.json b/internal/tsc_wrapped/package.json
index 2149c53..c30b905 100644
--- a/internal/tsc_wrapped/package.json
+++ b/internal/tsc_wrapped/package.json
@@ -7,8 +7,8 @@
"@types/tmp": "0.0.33",
"protobufjs": "5.0.0",
"tsickle": "0.25.5",
- "typescript": "2.5.3",
- "tsutils": "2.12.1",
+ "typescript": "2.6.2",
+ "tsutils": "2.20.0",
"tmp": "0.0.33"
}
}
diff --git a/internal/tsetse/error_code.ts b/internal/tsetse/error_code.ts
index ef1df60..321a716 100644
--- a/internal/tsetse/error_code.ts
+++ b/internal/tsetse/error_code.ts
@@ -1,4 +1,5 @@
export enum ErrorCode {
CHECK_RETURN_VALUE = 21222,
EQUALS_NAN = 21223,
+ BAN_EXPECT_TRUTHY_PROMISE = 21224,
}
diff --git a/internal/tsetse/rules/ban_expect_truthy_promise_rule.ts b/internal/tsetse/rules/ban_expect_truthy_promise_rule.ts
new file mode 100644
index 0000000..6f852ae
--- /dev/null
+++ b/internal/tsetse/rules/ban_expect_truthy_promise_rule.ts
@@ -0,0 +1,77 @@
+/**
+ * @fileoverview Bans expect(returnsPromise()).toBeTruthy(). Promises are always
+ * truthy, and this pattern is likely to be a bug where the developer meant
+ * expect(await returnsPromise()).toBeTruthy() and forgot the await.
+ */
+
+import * as tsutils from 'tsutils';
+import * as ts from 'typescript';
+
+import {Checker} from '../checker';
+import {ErrorCode} from '../error_code';
+import {AbstractRule} from '../rule';
+
+export class Rule extends AbstractRule {
+ readonly ruleName = 'ban-expect-truthy-promise';
+ readonly code = ErrorCode.BAN_EXPECT_TRUTHY_PROMISE;
+
+ register(checker: Checker) {
+ checker.on(
+ ts.SyntaxKind.PropertyAccessExpression, checkForTruthy, this.code);
+ }
+}
+
+function checkForTruthy(checker: Checker, node: ts.Node) {
+ const tc = checker.typeChecker;
+
+ if (!tsutils.isPropertyAccessExpression(node)) {
+ return;
+ }
+
+ if (node.name.getText() !== 'toBeTruthy') {
+ return;
+ }
+
+ const expectCallNode = getLeftmostNode(tc, node);
+ if (!ts.isCallExpression(expectCallNode)) {
+ return;
+ }
+
+ const signature = checker.typeChecker.getResolvedSignature(expectCallNode);
+ if (signature === undefined) {
+ return;
+ }
+
+ const symbol = tc.getReturnTypeOfSignature(signature).getSymbol();
+ if (symbol === undefined) {
+ return;
+ }
+
+ // Only look for methods named expect that return a Matchers
+ if (!((symbol.name === 'Matchers') &&
+ expectCallNode.expression.getText() === 'expect')) {
+ return;
+ }
+
+ if (!tsutils.isThenableType(tc, expectCallNode.arguments[0]) ||
+ tsutils.isAwaitExpression(expectCallNode.arguments[0])) {
+ return;
+ }
+
+ const argType = tc.getTypeAtLocation(expectCallNode.arguments[0]);
+ checker.addFailureAtNode(
+ node,
+ `Value passed to expect() is of type ${tc.typeToString(argType)}, which` +
+ ` is thenable. Promises are always truthy. Either use toBe(true) or` +
+ ` await the value.` +
+ `\n\tSee http://tsetse.info/ban-expect-truthy-promise`);
+}
+
+function getLeftmostNode(
+ tc: ts.TypeChecker, node: ts.PropertyAccessExpression) {
+ let current: ts.LeftHandSideExpression|undefined = node;
+ while (ts.isPropertyAccessExpression(current)) {
+ current = current.expression;
+ }
+ return current;
+}
diff --git a/internal/tsetse/runner.ts b/internal/tsetse/runner.ts
index c046dc7..2d77c21 100644
--- a/internal/tsetse/runner.ts
+++ b/internal/tsetse/runner.ts
@@ -9,6 +9,7 @@
import {Checker} from './checker';
import {AbstractRule} from './rule';
+import {Rule as BanExpectTruthyPromiseRule} from './rules/ban_expect_truthy_promise_rule';
import {Rule as CheckReturnValueRule} from './rules/check_return_value_rule';
import {Rule as EqualsNanRule} from './rules/equals_nan_rule';
@@ -19,6 +20,7 @@
const ENABLED_RULES: AbstractRule[] = [
new CheckReturnValueRule(),
new EqualsNanRule(),
+ new BanExpectTruthyPromiseRule(),
];
/**
diff --git a/internal/tsetse/tests/ban_expect_truthy_promise/jasmine_types.ts b/internal/tsetse/tests/ban_expect_truthy_promise/jasmine_types.ts
new file mode 100644
index 0000000..bcea24f
--- /dev/null
+++ b/internal/tsetse/tests/ban_expect_truthy_promise/jasmine_types.ts
@@ -0,0 +1,14 @@
+// Abbreviated Jasmine types for testing purposes
+
+declare function describe(
+ description: string, specDefinitions: () => void): void;
+declare function it(
+ expectation: string, assertion?: (done: Function) => void,
+ timeout?: number): void;
+declare function expect(actual: any): Matchers;
+
+interface Matchers {
+ toBeTruthy(expectationFailOutput?: any): boolean;
+ toBeFalsy(expectationFailOutput?: any): boolean;
+ not: Matchers;
+}
diff --git a/internal/tsetse/tests/ban_expect_truthy_promise/negatives.ts b/internal/tsetse/tests/ban_expect_truthy_promise/negatives.ts
new file mode 100644
index 0000000..62a3485
--- /dev/null
+++ b/internal/tsetse/tests/ban_expect_truthy_promise/negatives.ts
@@ -0,0 +1,10 @@
+function expect(x: {}): ClassWithTruthy {
+ return new ClassWithTruthy();
+}
+
+class ClassWithTruthy {
+ toBeTruthy() {}
+}
+
+new ClassWithTruthy().toBeTruthy();
+expect(Promise.resolve(1)).toBeTruthy();
diff --git a/internal/tsetse/tests/ban_expect_truthy_promise/positives.ts b/internal/tsetse/tests/ban_expect_truthy_promise/positives.ts
new file mode 100644
index 0000000..6ec611f
--- /dev/null
+++ b/internal/tsetse/tests/ban_expect_truthy_promise/positives.ts
@@ -0,0 +1,53 @@
+function returnsPromise() {
+ return Promise.resolve(true);
+}
+
+type Future = Promise<number>;
+function aliasedPromise(): Future {
+ return Promise.resolve(1);
+}
+
+class Extended extends Promise<number> {}
+function extendedPromise(): Extended {
+ return Promise.resolve(1);
+}
+
+class DoubleExtended extends Extended {}
+function doubleExtendedPromise(): Extended {
+ return Promise.resolve(1);
+}
+
+function maybePromise(): Promise<number>|number {
+ return 3;
+}
+
+describe('example test', () => {
+ it('should error when expecting promises toBeTruthy', async () => {
+ const promise = returnsPromise();
+
+ // Every expect here should fail the check.
+ expect(promise).toBeTruthy();
+ expect(maybePromise()).not.toBeTruthy();
+ expect(maybePromise()).toBeTruthy();
+ expect(returnsPromise()).toBeTruthy();
+ expect(aliasedPromise()).toBeTruthy();
+ expect(extendedPromise()).toBeTruthy();
+ expect(doubleExtendedPromise()).toBeTruthy();
+ });
+
+ it('should allow awaited promises', async () => {
+ const promise = returnsPromise();
+ expect(await promise).toBeTruthy();
+ expect(await maybePromise()).toBeTruthy();
+ expect(await returnsPromise()).toBeTruthy();
+ expect(await aliasedPromise()).toBeTruthy();
+ expect(await extendedPromise()).toBeTruthy();
+ });
+
+ it('should only error for promises', async () => {
+ expect('truthy').toBeTruthy();
+ expect(1).toBeTruthy();
+ expect(true).toBeTruthy();
+ expect(null).not.toBeTruthy();
+ });
+});
diff --git a/internal/tsetse/tsconfig.json b/internal/tsetse/tsconfig.json
index a5f8cb1..e59aeac 100644
--- a/internal/tsetse/tsconfig.json
+++ b/internal/tsetse/tsconfig.json
@@ -9,7 +9,8 @@
"dom",
"es5",
"es2015.core",
- "es2015.collection"
+ "es2015.collection",
+ "es2015.promise"
],
"plugins": [
// Run `bazel build internal/tsetse:language_service_plugin` for this to work.
diff --git a/package.json b/package.json
index b973398..41a0693 100644
--- a/package.json
+++ b/package.json
@@ -22,8 +22,8 @@
"protobufjs": "5.0.0",
"protractor": "^5.2.0",
"tsickle": "0.25.x",
- "tsutils": "2.12.1",
- "typescript": "2.5.x"
+ "tsutils": "2.20.0",
+ "typescript": "2.6.x"
},
"scripts": {
"pretest": "webdriver-manager update && bazel build examples/app:all",
