tests(pubsub): integrate pub/sub sample system tests that are still useful by feywind · Pull Request #8091 · googleapis/google-cloud-node

feywind · 2026-04-21T22:19:37Z

Extracts system tests from the sample system tests in the original repo, and adds those to the handwritten/pubsub tree.

gemini-code-assist

Code Review

This pull request introduces a comprehensive testing framework for Pub/Sub, including unit tests for common utilities and system tests for core operations like topic creation and message publishing. It also adds a TestResources class to manage resource lifecycle and prevent naming conflicts. Key feedback includes addressing a potential memory leak in the message listener timeout logic, improving the testability of the cleanup utility by using the provided timestamp provider instead of a direct call to Date.now(), and removing redundant any type assertions for better type safety.

feywind · 2026-04-28T20:51:07Z

This is obviously not running.

Checking for changes in handwritten/pubsub...
No changes detected in handwritten/pubsub. Skipping tests.

feywind · 2026-06-02T23:45:43Z

This is obviously not running.

Checking for changes in handwritten/pubsub...
No changes detected in handwritten/pubsub. Skipping tests.

The tests are running again now that it's moved to GCB.

feywind · 2026-06-25T19:58:14Z

@shivanee-p Unfortunately this still requires some work to get CI passing. Maybe this is a hole in the new assigner script? Or was reassigning existing issues part of the initial cleanup pass?

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

feywind · 2026-07-09T22:42:40Z

The old branch had unfortunately become very crufty from merges, so I refactored it.

westarle · 2026-08-13T19:31:05Z

+
+    const message = await new Promise<Message>((resolve, reject) => {
+      const timeout = setTimeout(() => reject(new Error('Timeout waiting for Avro record')), 15000);
+      subscription.once('message', (m: Message) => {


Do we need to remove this listener if we hit timeout (like you did in pubsub.ts?)

Sorry I didn't point it out before, there are several cases like this remaining in the PR. Can you please address those, too?

westarle · 2026-08-13T19:34:50Z

    "@types/tmp": "^0.2.6",
+    "avro-js": "^1.12.1",
    "c8": "^10.1.3",
+    "chai": "^6.2.2",


do we use chai consistently in our tests?

If we can workaround features of built-in assert, maybe we should stick to that.

Generally, I'd prefer to have consistent APIs across tests. Can you consider a follow-up PR to move all the assertions to chai, if that's the best thing?

We used to, we don't anymore - we're trying to move to the simpler built-in assert. I'll remove this.

westarle · 2026-08-13T19:37:39Z

+// I don't like that these two files (this plus ".test") are duplicated
+// across the two test structures, but because of the tangle of rootDirs
+// and package.json "files", it's hard to avoid it.


I like the idea of testresources, is there a way we can avoid the duplication? Could this be put in some shared test-utils dependency?

There's not really a good candidate right now for a separate library - I like the idea of making a test-utils in core somewhere. Especially in goofy cases like this. But it generally seems useful for the handwritten libraries for their system tests.

I'm OK if its a follow-up, please file a ticket to dedupe these.

This is b/554057686.

westarle · 2026-08-13T19:39:18Z

 import * as opentelemetry from '@opentelemetry/api';
 import {google} from '../protos/protos';
 import * as defer from 'p-defer';
+import * as crypto from 'node:crypto';


This is great! Can you do this to other imports, here or in another PR?

You mean the adding node:, right? I agree it's good practice now. I'll make a separate PR in a bit.

westarle · 2026-08-14T11:32:46Z

+// I don't like that these two files (this plus ".test") are duplicated
+// across the two test structures, but because of the tangle of rootDirs
+// and package.json "files", it's hard to avoid it.


I'm OK if its a follow-up, please file a ticket to dedupe these.

westarle · 2026-08-14T11:35:09Z

+
+    const message = await new Promise<Message>((resolve, reject) => {
+      const timeout = setTimeout(() => reject(new Error('Timeout waiting for Avro record')), 15000);
+      subscription.once('message', (m: Message) => {


Sorry I didn't point it out before, there are several cases like this remaining in the PR. Can you please address those, too?

westarle · 2026-08-14T11:36:09Z

please address all the remaining listeners before submitting.

feywind self-assigned this Apr 21, 2026

feywind added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 21, 2026

product-auto-label Bot added the samples Issues that are directly related to samples. label Apr 21, 2026

feywind marked this pull request as draft April 21, 2026 22:19

gemini-code-assist Bot reviewed Apr 21, 2026

View reviewed changes

Comment thread handwritten/pubsub/system-test/sample-tests.ts Outdated

Comment thread handwritten/pubsub/system-test/testResources.ts Outdated

Comment thread handwritten/pubsub/system-test/sample-tests.ts Outdated

feywind marked this pull request as ready for review April 28, 2026 20:19

feywind removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 28, 2026

feywind requested a review from a team as a code owner May 1, 2026 21:21

feywind closed this Jun 1, 2026

feywind reopened this Jun 1, 2026

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 2, 2026

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 2, 2026

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 3, 2026

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 3, 2026

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 3, 2026

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 5, 2026

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 8, 2026

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 8, 2026

feywind mentioned this pull request Jun 8, 2026

build(pubsub): bring over build fixes from the samples PR #8455

Merged

feywind changed the title ~~tests: integrate pub/sub sample system tests that are still useful~~ tests(pubsub): integrate pub/sub sample system tests that are still useful Jun 8, 2026

shivanee-p removed the request for review from a team June 22, 2026 21:16

shivanee-p unassigned feywind Jun 22, 2026

shivanee-p requested a review from pearigee June 22, 2026 21:16

feywind self-assigned this Jun 25, 2026

feywind and others added 3 commits July 9, 2026 18:27

tests: integrate sample system tests that are still useful

fa8d8b3

chore: remove an unnecessary any

5a292f3

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

tests: integrate more sample tests

dd90f8e

feywind added 8 commits July 9, 2026 18:33

chore: add back in crypto imports

1bc72df

chore: fix(?) auth typing

9ea19a2

fix: a bunch of pnpm breakages

e4d5220

tests: clean up generated avro/proto system tests

ea1b956

chore: add copyright header

0d50d95

tests: small fix from avro updates

eb26949

tests: shut down otel provider after system test

4d9cf12

fix: refactor error fix for crypto

b215ee1

feywind force-pushed the feywind-pubsub-samples branch from ca5cc15 to b215ee1 Compare July 9, 2026 22:42

feywind added 3 commits August 12, 2026 16:44

chore: merge from main

b3d34be

chore: revert merged version changes

f3d4570

chore: typo

ff93f81

feywind removed the request for review from pearigee August 12, 2026 20:51

feywind added 3 commits August 12, 2026 17:34

chore: revert the revert, do the span warp again

a9ed76b

tests: fix test failures in avro and otel samples

b31f20f

fix: revert logging changes, the other ones were correct

179c0b5

feywind enabled auto-merge (squash) August 13, 2026 19:10

chore: poke CI

2828510

westarle reviewed Aug 13, 2026

View reviewed changes

feywind disabled auto-merge August 13, 2026 20:13

feywind added 3 commits August 13, 2026 17:17

fix: remove message listener on timeout in avro-samples test

f8aed45

tests: remove chai

dc86d86

chore: remove unnecessary no-op changes from merging

a1e67d8

westarle approved these changes Aug 14, 2026

View reviewed changes

feywind added 2 commits August 28, 2026 16:04

tests: factor out subscription waiting to catch and remove handlers

4b011c2

chore: fix new linter errors

eee9fc9

feywind merged commit 093572e into main Aug 28, 2026
50 checks passed

feywind deleted the feywind-pubsub-samples branch August 28, 2026 20:38

Uh oh!

Conversation

feywind commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feywind commented Apr 28, 2026

Uh oh!

feywind commented Jun 2, 2026

Uh oh!

feywind commented Jun 25, 2026

Uh oh!

feywind commented Jul 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feywind Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westarle commented Aug 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feywind commented Apr 21, 2026 •

edited

Loading

feywind Aug 13, 2026 •

edited

Loading