First stab at adding support for tests timeout

[DmitrySharabin]

No description provided.

[netlify]

✅ Deploy Preview for h-test ready!

Name	Link
🔨 Latest commit	bd83528
🔍 Latest deploy log	https://app.netlify.com/sites/h-test/deploys/67b73bc7e73d960008801e15
😎 Deploy Preview	https://deploy-preview-80--h-test.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[LeaVerou]

Wait why are we adding another option instead of fixing maxTime?

[DmitrySharabin]

Wait why are we adding another option instead of fixing maxTime?

After thinking about this a bit more, I agree that we probably don't need another global option.

I see some use cases that should be considered, though.

• If a test has expect or throws (i.e., it's not a time-based test), we can use maxTime as a timeout time (since it doesn't have any other meaning in this case).
• However, if we have a time-based test, we might interpret maxTime/maxTimeAsync wrong: it shouldn't be considered a timeout time. The test might take more than maxTime/maxTimeAsync to run. So, it should be considered failing but not because it timed out. That means we need another way to set a timeout time for that case.

The more I think about it, the more I lean towards not re-purposing maxTime to set timeout time, and we need a separate property for it. What do you think?

[LeaVerou]

Wait why are we adding another option instead of fixing maxTime?

After thinking about this a bit more, I agree that we probably don't need another global option.

I see some use cases that should be considered, though.

• If a test has expect or throws (i.e., it's not a time-based test), we can use maxTime as a timeout time (since it doesn't have any other meaning in this case).
• However, if we have a time-based test, we might interpret maxTime/maxTimeAsync wrong: it shouldn't be considered a timeout time. The test might take more than maxTime/maxTimeAsync to run. So, it should be considered failing but not because it timed out. That means we need another way to set a timeout time for that case.

The more I think about it, the more I lean towards not re-purposing maxTime to set timeout time, and we need a separate property for it. What do you think?

I don't understand. I would expect that if a test has maxTime and no throws or expect that it would fail if it exceeds maxTime. What actually happens?

[DmitrySharabin]

I don't understand. I would expect that if a test has maxTime and no throws or expect that it would fail if it exceeds maxTime. What actually happens?

For now, if a test has maxTime/maxTimeAsync, it would fail if it exceeds the specified time if only the test has actually finished running. But if something prevents it from being finished (e.g., we await some promise that will never be fulfilled), the test won't be evaluated at all because this line will never be reached:

htest/src/classes/TestResult.js

Line 118 in 6e988f1

this.evaluate();

Now I see the actual bug that should be fixed and why it didn't work when I tried limiting the time for test execution. Hm. Let me update the code to address it.

[DmitrySharabin]

It turned out that setting a timeout via setTimeout is not enough: when the specified time runs out, we don't break the test execution, but should. After some research, I thought that abortable API is our go-to solution. I implemented it by making the interceptConsole() function abortable (we execute tests in the body of this function).

This is my first time working with AbortSignal, so I might have missed something or made it wrong. I'm open to any feedback.

[LeaVerou]

It turned out that setting a timeout via setTimeout is not enough: when the specified time runs out, we don't break the test execution, but should. After some research, I thought that abortable API is our go-to solution. I implemented it by making the interceptConsole() function abortable (we execute tests in the body of this function).

This is my first time working with AbortSignal, so I might have missed something or made it wrong. I'm open to any feedback.

At first glance, this seems like a very complicated solution to this problem. If the issue is that we need to externally reject the promise, we can perhaps wrap the promise and do something like this or use Promise.withResolvers() (though the former is more well supported, and not much more code)

[DmitrySharabin]

we can perhaps wrap the promise and do something like this

Oh, that's a very nice idea. Let me try to use it. By the way, I started to recognize this pattern in our projects. It's time to internalize it. 🙂

[DmitrySharabin]

After some more trials and errors, I think I could find exactly where set setTimeout should be used to support the logic of aborting test execution after time out (and setting all the required properties)—no need to externally resolve/reject a promise and (moreover) in using AbortSignal.

Could you please give this PR another look?

[DmitrySharabin]

After some more trials and errors, I think I could find exactly where set setTimeout should be used to support the logic of aborting test execution after time out (and setting all the required properties)—no need to externally resolve/reject a promise and (moreover) in using AbortSignal.

It's still not enough. 🤦‍♂️ Even though a promise resolved on timeout might have precedence, the promise running the test is not aborted. When it is, it updates some properties of the test that should not be updated, such as this.timeTaken. We can see this in the summary after all the tests have run.