Skip to content

tests: integrate pub/sub sample system tests that are still useful#8091

Draft
feywind wants to merge 3 commits intomainfrom
feywind-pubsub-samples
Draft

tests: integrate pub/sub sample system tests that are still useful#8091
feywind wants to merge 3 commits intomainfrom
feywind-pubsub-samples

Conversation

@feywind
Copy link
Copy Markdown
Contributor

@feywind feywind commented Apr 21, 2026

Work in progress for now.

@feywind feywind self-assigned this Apr 21, 2026
@feywind 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 product-auto-label Bot added the samples Issues that are directly related to samples. label Apr 21, 2026
@feywind feywind marked this pull request as draft April 21, 2026 22:19
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +78 to +85
const message = await new Promise<Message>((resolve, reject) => {
const timeout = setTimeout(() => reject(new Error('Timeout')), 10000);
subscription.once('message', (m: Message) => {
clearTimeout(timeout);
m.ack();
resolve(m);
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The 'message' listener is not removed if the promise rejects due to a timeout. This can lead to memory leaks or unexpected behavior in the test suite if a message arrives after the timeout. It is better to explicitly remove the listener in the timeout path.

    const message = await new Promise<Message>((resolve, reject) => {
      const timeout = setTimeout(() => {
        subscription.removeListener('message', messageHandler);
        reject(new Error('Timeout'));
      }, 10000);
      function messageHandler(m: Message) {
        clearTimeout(timeout);
        m.ack();
        resolve(m);
      }
      subscription.once('message', messageHandler);
    });

if (name.startsWith(this.testSuiteId)) {
const parts = name.split('-');
const createdAt = Number(parts[1]);
const timeDiff = (Date.now() - createdAt) / (1000 * 60 * 60);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use this.tokenMaker.timestamp() instead of Date.now() to ensure consistency with the rest of the class and to allow for deterministic testing of the cleanup logic. The current implementation makes the unit tests dependent on the system clock.

Suggested change
const timeDiff = (Date.now() - createdAt) / (1000 * 60 * 60);
const timeDiff = (this.tokenMaker.timestamp() - createdAt) / (1000 * 60 * 60);

Comment thread handwritten/pubsub/system-test/sample-tests.ts Outdated
feywind and others added 2 commits April 21, 2026 18:24
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Indicates a pull request not ready for merge, due to either quality or timing. samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant