Set timeout + streaming logs for CI tests

[ajeetdsouza]

We recently had a 6-hour CI run due to a hung test: https://github.com/connectrpc/connect-es/actions/runs/33697561334/job/100469649229

turbo captures test output, so if the it gets killed because it hung, we have no error logs to work with. To prevent this from happening, I added --log-order stream, which will mix up the outputs, but will give us some information to work with in cases like these.

[timostamm]

turbo captures test output, so if the it gets killed because it hung, we have no error logs to work with.

Can you try running turbo with --log-order=stream? If this works well enough, maybe we don't need --test-timeout in every package.json.

[ajeetdsouza]

I did consider log-order=stream, but that will mix up unrelated logs, making everything harder to read. The test timeout ensures that tests stay under 2min, which IMO is a good thing to have anyway.

[timostamm]

I did consider log-order=stream, but that will mix up unrelated logs, making everything harder to read. The test timeout ensures that tests stay under 2min, which IMO is a good thing to have anyway.

Really unfortunate that Node's test runner doesn't apply a sensible default timeout :(

But I'm not convinced that adding a --test-timeout=120000 to every node --test is the right solution. Do we apply it just for some packages, or for all of them? I don't think connect-migrate and connect-express needs it. Is it something we keep forever, or just until we get the tests in better shape?

For log-order=stream, see:

connect-es/.github/workflows/conformance-node.yaml

Line 39 in d119f23

- run: npx turbo run conformance:${{matrix.side}} --filter '@connectrpc/connect-node' --output-logs new-only --log-order stream

Not applicable to

connect-es/.github/workflows/ci.yaml

Line 73 in d119f23

- run: npx turbo run test

?

[ajeetdsouza]

Do we apply it just for some packages, or for all of them?

If we agree that a default timeout is valuable and is something that should have been included in Node.js, I would say we should apply a timeout in all packages, whether or not we ship any changes related to turbo logging.

For log-order=stream, see:

The problem with --log-order=stream is that it mixes up the output from turbo tasks that are running in parallel. This means that npx turbo run test would mix up the outputs of @connectrpc/connect:test, @connectrpc/connect-migrate:test, etc.

The reason we don't have this problem with our conformance tests is that conformance:server and conformance:client run in different jobs using a CI matrix, so nothing runs in parallel within a job. This would add a lot of overhead if we start spinning up jobs for each package's tests. The other alternative is to disable parallelism (npx run turbo test --log-order=stream --concurrency=1) but that would also slow down CI.

[timostamm]

Do we apply it just for some packages, or for all of them?

If we agree that a default timeout is valuable and is something that should have been included in Node.js, I would say we should apply a timeout in all packages, whether or not we ship any changes related to turbo logging.

We agree that Node.js should have had a default timeout for --test, but I don't agree we should add the --test-timeout flag to every package.

The problem we want to solve is that some tests do I/O. They aren't written defensively, and can get stuck. CI won't show us what got stuck because turbo buffers until the script exits.

--test-timeout doesn't fix this. Node.js reports test results after the timeout, but only v22 exits. To exit on newer versions, we would have to also add the --test-force-exit flag.

--test-force-exit masks handle leaks by design, and I've seen it swallow unhandled promise rejections. Two important signals for us, so we can't use it.

From what I see, turbo with --log-order stream is the only reliable way to get logs in CI for hangy tests. I expect turbo to write line by line, and to prefix all lines with the package name, so it's not completely terrible.

--test-timeout in combination with --log-order stream is helpful, but I don't think it should be added to every package. The flag has a different meaning across Node versions, and copying the constant to every package isn't great. I would add the flag where hangs are plausible, not everywhere.

[ajeetdsouza]

The flag has a different meaning across Node versions

TIL that --test-timeout=120000 means 2 minutes per-test on Node 24 / 26, and 2 minutes per-file on Node 22.

Thanks for the heads up! I've switched to --log-order=stream and updated the PR title / description.

[timostamm]

Thanks for the perseverance, pitfalls all around here 😢