Google-cloud-node: Remove --no-timeouts from all tests

[JustinBeckwith]

As it would turn out, the --no-timeouts option in Mocha often leads to tests just exiting early with a 0 exit code:
https://github.com/mochajs/mocha/issues/2537

Looking through the googleapis org, it looks like this is done all over the place:
https://github.com/search?q=org%3Agoogleapis+--no-timeouts&type=Code

We need to track these down, and update them to not use that flag.

[JustinBeckwith]

@GoogleCloudPlatform/node-team FYI.

[JustinBeckwith]

@alexander-fenster @jmdobry @lukesneeringer this is kind of an example where having all separate repos is sorta rough. How do you propose we make broad changes like this across all of our package.json?

[alexander-fenster]

That's the one main reason why I spent some time on github-repo-automation. This particular change can be easily made using lib/update-repo.js.

[alexander-fenster]

Or even better, lib/update-file.js as it's just one JSON file to update.

[stephenplusplus]

This hasn't been a problem for us before, and that bug doesn't sound that buggy. I know we are getting this behavior when we upgrade to the latest @google-cloud/[email protected], but I did a quick test without no-timeouts, but with timeout=20000 (as https://github.com/googleapis/nodejs-storage/pull/195 proposes), and I still get errors:

  storage
    without authentication
      1) "before all" hook
    acls
      buckets
(node:10488) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): AssertionError [ERR_ASSERTION]: false == true
(node:10488) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Maybe there's an underlying issue somewhere else in the stack? Or maybe I just need more context to understand.

[kjin]

@stephenplusplus That issue may be related to https://github.com/googleapis/nodejs-common/issues/137.

[JustinBeckwith]

There's a trail of sadness that led to us figuring this out, sure. The point is - it's a real problem if I can introduce a bug that causes all of our tests to not run, and still return a 0 exit code :) . I had submitted a PR with "passing" unit tests, even though over half of them never ran because of an early exit.

[stephenplusplus]

I had submitted a PR with "passing" unit tests, even though over half of them never ran because of an early exit.

That shouldn't have happened. Unit tests should be using the default mocha timeout of 2000ms.

it's a real problem if I can introduce a bug that causes all of our tests to not run, and still return a 0 exit code :)

It's happening in system tests, so it's not as critical IMO. Before we merge a PR, the reviewer / merger should be running the system tests to be sure a bug wasn't introduced. (And of course, before sending the PR, but that's easy to forget sometimes)

We've learned over the years that different projects require different values for timeouts, e.g. Compute with all of the creation/deletion operations. We started at 5000ms, and went up as necessary. Removing the timeout was easiest-- the build will kill the process after something like 10 minutes, anyway. Considering that, I'm okay with setting a super high timeout, i.e. several minutes.

[JustinBeckwith]

That shouldn't have happened. Unit tests should be using the default mocha timeout of 2000ms.

Sadly, it's the case today though 😰 It looks like we're using --no-timeouts on unit tests as well as system tests.

It's happening in system tests, so it's not as critical IMO. Before we merge a PR, the reviewer / merger should be running the system tests to be sure a bug wasn't introduced.

I disagree on two fronts :)

1. It's easy to not notice this bug is happenning. In my case, ~200 tests passed, and it exited with a 👍. From my perspective - looked great! I had no idea there were over 400 tests.

2. The fact that we're not running system-tests on PRs makes me a different kind of nervous. I understand the reasons, and I don't have a good solution so I won't poke at this too much :)

To be clear - I have no problem with a --timeout 6.022x10^23, set it as high as you want 😆. I do have a problem with exiting with a 0 exit code while not running half the tests.

[stephenplusplus]

Spoiler: I agree that we should set a high limit. The following is only for clarity, in the event that 2 years from now, someone asks "What's up with the super high timeouts?" :)

Judging from only the first page of results, it looks like only Storage set --no-timeouts on unit tests. I recall some Node v4 CI builds stalling for various APIs, so that could be why we have exception(s).

The 200 tests passing thing shouldn't have happened, but that's a different bug. That's "unit tests need a timeout", as opposed to "system tests need a timeout". If we unified the test procedure across the APIs to restore the default mocha timeout, that would prevent future CI false positives.

I hope we can run system-tests on PRs as well, so, sure, if we ever find a way, this issue could repeat itself. It could also be that by that time, there was already a patch in Mocha, or somewhere else, to address this. But, I don't mind erring on the side of caution.

[JustinBeckwith]

@alexander-fenster is this something we could do with your automation tool?

[alexander-fenster]

Done a few weeks ago, with the automation tool of course :)
list of commits