Don't await closing clients

[ejizba]

Occasionally seeing this error in the tests

eventHub
       "after all" hook in "eventHub":
     Error: Timeout of 180000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/1/s/out/eventHub.test.js)
      at listOnTimeout (node:internal/timers:569:17)
      at process.processTimers (node:internal/timers:512:7)

But there's no particular reason we need to wait for these clients to close

[castrodd]

But there's no particular reason we need to wait for these clients to close

Wouldn't it eventually be problematic if these clients are never properly closed? I feel a little nervous about that. Maybe we could do something like this to get some insight into the underlying issue?

await Promise.all([
        clientOneTriggerAndOutput.close(),
        clientOneTrigger.close(),
        clientManyTriggerAndOutput.close(),
        clientManyTrigger.close(),
    ]).catch(err => {
        console.error("Error:", err);
    });

I defer to your judgment, but it seems like this could come back to bite us if we don't figure out why .close() is timing/erroring out. Let me know what you think.

[castrodd]

I left a comment, but trust whichever option you think is best.

[ejizba]

Wouldn't it eventually be problematic if these clients are never properly closed? I feel a little nervous about that.

If this was production code, then yes we should be careful. The node process in production can be very long-running and use a lot of resources, so properly cleaning up those resources is important to prevent it from getting in a bad state. In addition, any failures in production are a big deal for our customers.

For test code, the process is short-running (at most an hour in Azure Dev Ops), creates a known amount of resources (since we're not running customer code), and overall has very little impact if it fails. We're the only people affected by flaky test failures and at-most it causes us to waste time investigating the failure or re-running the build.

Maybe we could do something like this to get some insight into the underlying issue? It seems like this could come back to bite us if we don't figure out why .close() is timing/erroring out.

I've never seen any exceptions from closing clients, aka I don't have any indication that it's "erroring out". The known issue right now is timeouts, which is a common problem for end-to-end tests running against real Azure resources. Unit tests should only take milliseconds, but end-to-end tests will often take seconds or even minutes by their very nature.

Production code and test code should be treated/reviewed very differently IMO. For test code, I normally avoid commenting nits when I'm reviewing or updating based on nits when I'm the reviewee. I believe this speeds up the process and encourages people to write tests more often, which is a goal of mine since I'm a big fan of tests

[castrodd]

If this was production code, then yes we should be careful. The node process in production can be very long-running and use a lot of resources, so properly cleaning up those resources is important to prevent it from getting in a bad state. In addition, any failures in production are a big deal for our customers.

For test code, the process is short-running (at most an hour in Azure Dev Ops), creates a known amount of resources (since we're not running customer code), and overall has very little impact if it fails. We're the only people affected by flaky test failures and at-most it causes us to waste time investigating the failure or re-running the build.

Thank you for the detailed explanation. That all makes sense to me. I approved the PR earlier, so feel free to merge. I have no other questions or concerns. I just wanted to double-check that I understood the reasoning behind your changes.