Add option to throw when passing expect a normal Promise

[andrewbranch]

Are you requesting a feature or reporting a bug?

Feature request, sort of. Also, I’m willing to do the work myself.

What is the current behavior?

Internally, TestCafe uses a class called ClientFunctionResultPromise that extends Promise to allow ClientFunctions to be retried in the Smart Assertion Query Mechanism. Most properties on Selector instances return these. It's super useful to pass these to t.expect so they get retried until they pass, or a timeout passes, e.g.

const element = Selector('#element');
const thingIsVisible = element.visible; // This is a ClientFunctionResultPromise
await t.expect(thingIsVisible).ok({ timeout: 1000 });

However, this can also lead to confusion, because if you pass a normal Promise to t.expect(), it will not be awaited or retried; it will use the Promise object itself as the value being asserted. And it’s very easy to fall into this trap, because chaining a .then on a ClientFunctionResultPromise returns a normal Promise:

const element = Selector('#element');
const thingIsVisible = element().then(snapshot => snapshot.visible); // This is a Promise
await t.expect(thingIsVisible).ok({ timeout: 1000 });

This looks fairly similar, but the assertion always passes, even if the thing is never visible, because the Promise object is truthy. So the big problem is that the mistake of passing a normal Promise to t.expect() is really harmful with .ok(). For any other assertion, you’re likely to catch your mistake, but with .ok(), it’s very easy to miswrite an assertion that passes when it shouldn’t.

To make matters worse for those of us who are using TypeScript, the type definitions for TestCafe don’t expose ClientFunctionResultPromise; instead, it types every ClientFunctionResultPromise as a normal Promise. So these two similar-looking examples above actually look identical to the type system, making this mistake impossible to lint for or guard against with static typings.

What is the expected behavior?

Two proposals:

1. It seems like passing a normal Promise to t.expect() is probably a mistake >99% of the time. Can we somehow add a configuration option that would cause t.expect(normalPromise) to throw an error? If that’s too presumptive, what about making t.expect(normalPromise).ok() throw an error? It could perhaps be bypassed with something like .ok({ allowUnawaitedPromises: true })?

2. If the type definitions exposed ClientFunctionResultPromise, we could at least lint for this, or write a type-safe wrapper for t.expect or something. Only catch with this is that because TypeScript uses structural typing rather than nominal typing, ClientFunctionResultPromise would look identical to Promise unless the added methods were made public on the type, and they look like they’re meant to be private. There are some possible hacks around this, but they’d still surface awkwardly in the type system. I think my first proposal is better.

Would you accept a PR for either of these ideas? If the former sounds reasonable, what are your thoughts on an API for it?

[kirovboris]

Hi @andrewbranch,

Yes, this behavior can cause a false positive result.

I like the first approach. I think we could throw a warning for Promise inside of .expect() and ignore it with an assertion option like

await t
    .expect(getResult())
    .ok({ allowUnawaitedPromises: true //false by default })

@AndreyBelym @AlexanderMoskovkin what do you think?

[andrewbranch]

Hi folks, any thoughts on this? Should I get started, or wait for @AndreyBelym and @AlexanderMoskovkin to weigh in? Thanks for your time!

[adrianaisemberg]

@kirovboris when a warning is thrown, how does it affect the running test?
How does it affect the reports?
If it fails the test (which IMHO it should) - then it's not a warning, is it?

Regarding your suggestion to add a flag (allowUnawaitedPromises) - if such flag is added, I think there should also be a way to toggle it globally (via command-line or config file) rather than only per-assertion.

[kirovboris]

@adrianaisemberg
Yes, I think an error would be a better choice.
Also I don't see any cases we need the global flag. Using a regular Promise in t.expect is a bad practice and a very rare case when you actually want to check

t.expect(callAsyncFn()).eql(smth);

i.e. compare Promise === smth

[AlexanderMoskovkin]

Hi @andrewbranch,

I feel your proposal is awesome! Let's do it =)

It seems like passing a normal Promise to t.expect() is probably a mistake >99% of the time. Can we somehow add a configuration option that would cause t.expect(normalPromise) to throw an error? If that’s too presumptive, what about making t.expect(normalPromise).ok() throw an error? It could perhaps be bypassed with something like .ok({ allowUnawaitedPromises: true })?

I vote for it. And I think it's actual not only for the ok assertion but for others too. It will show you a
comprehensible error if you add a native Promise to expect().eql() assertion (for example) by mistake.

I'm not sure about your second proposal. If you do it the TypeScript compiler will throw an error case regardless whether do you use the allowUnawaitedPromises option or not. So it will not work if you want to set allowUnawaitedPromises: true.

If it fails the test (which IMHO it should) - then it's not a warning, is it?

@adrianaisemberg, yep, @kirovboris means throw an error, not a warning.

Regarding your suggestion to add a flag (allowUnawaitedPromises) - if such flag is added, I think there should also be a way to toggle it globally (via command-line or config file) rather than only per-assertion.

This option is too specific and as @andrewbranch mentioned above It seems like passing a normal Promise to t.expect() is probably a mistake >99% of the time. I agree with this. I guess it's a very rare cases when you need to check a Promise with the t.expect().ok() (I can't imagine a real case for this. If you have, let me know). So I'm not sure this option is worth adding to the test-run level api.

[andrewbranch]

Great! I’ll get started! I feel like I have a good lead on where to begin the implementation, but I'm having trouble navigating the test structure. @AlexanderMoskovkin, where is the right place to test this new feature?

[AlexanderMoskovkin]

I guess it should be functional tests, you can add them to the existent assertions tests. To see how it works take a look at the test-functional-local gulp task. It runs mocha tests that run testcafe tests inside =) Each functional test works in the following way: mocha triggers a testcafe test, testcafe runs the test and returns some report, mocha checks the report.
For example add it.only to this test and run the gulp task.

Additionally it should be one more unit test that will check the formatting of the new created error (see here).

[andrewbranch]

One more question: how would you detect Promises that aren’t ClientFunctionResultPromises? I noticed that ClientFunctionResultPromise is implemented with the polyfill Pinkie, so expect could easily receive an instance of that, an instance of a native Promise, or an instance of a different polyfill employed by the user. Should we just check to see if the object is thennable? Or is it sufficient to check for instanceof PinkiePromise and instanceof Promise?

[andrewbranch]

Additionally it should be one more unit test that will check the formatting of the new created error

Are you saying I should make a new subclass of error for this? What should the base class be? Do you have ideas of what info should be included alongside the error message?

[AlexanderMoskovkin]

Should we just check to see if the object is thennable?

I guess it's ok.

What should the base class be? Do you have ideas of what info should be included alongside the error message?

In you PR you implement exactly what do I mean

[lock]

This thread has been automatically locked since it is closed and there has not been any recent activity. Please open a new issue for related bugs or feature requests. We recommend you ask TestCafe API, usage and configuration inquiries on StackOverflow.