Skip to content

test: migrate integration tests and vscode extension tests (Part 3)#10725

Open
joehan wants to merge 1 commit into
refactor/migrate-to-native-fetch-part2from
refactor/migrate-to-native-fetch-part3
Open

test: migrate integration tests and vscode extension tests (Part 3)#10725
joehan wants to merge 1 commit into
refactor/migrate-to-native-fetch-part2from
refactor/migrate-to-native-fetch-part3

Conversation

@joehan

@joehan joehan commented Jun 25, 2026

Copy link
Copy Markdown
Member

Description

This is Part 3 of the native fetch migration. It contains the integration and VS Code extension tests.

  • Added requireAuthWrapper(false) in the VS Code extension test runner (firebase-vscode/src/test/runTest.ts) to initialize the CLI auth state and prevent unhandled token refresh rejections.
  • Migrated the VS Code error handling integration tests to use the custom mock helper and set the token in the env.
  • Migrated integration test scripts (scripts/hosting-tests/, etc.).

This PR is stacked on top of:

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

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.

Code Review

This pull request cleans up unused node-fetch imports, adds a --debug flag to local hosting tests, and updates VS Code integration tests to use a temporary user data directory and mock authentication via nock. Feedback on these changes highlights a cross-platform compatibility issue with hardcoding /tmp on Windows, and recommends properly saving and restoring process.env.FIREBASE_TOKEN in tests to prevent test pollution.

Comment thread firebase-vscode/src/test/runTest.ts Outdated
const extensionTestsPath = path.resolve(__dirname, "./suite/index");

// Download VS Code, unzip it and run the integration test
const tmpUserData = path.join("/tmp", `vsc-ud-${Math.random().toString(36).substring(2, 7)}`);

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

Hardcoding "/tmp" as the temporary directory path will fail on Windows platforms, as /tmp is not a standard directory there. Use os.tmpdir() to resolve the temporary directory in a cross-platform manner.

Suggested change
const tmpUserData = path.join("/tmp", `vsc-ud-${Math.random().toString(36).substring(2, 7)}`);
const tmpUserData = path.join(require("os").tmpdir(), `vsc-ud-${Math.random().toString(36).substring(2, 7)}`);

authStub = stub(auth, "getAccessToken").resolves({
access_token: "an_access_token",
});
process.env.FIREBASE_TOKEN = "mock_refresh_token";

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

Modifying process.env.FIREBASE_TOKEN directly without restoring its original value can cause side effects or failures in other tests running in the same process (especially in CI environments where a real token might be set). Consider saving the original value and restoring it in teardown.

      (globalThis as any).originalFirebaseToken = process.env.FIREBASE_TOKEN;
      process.env.FIREBASE_TOKEN = "mock_refresh_token";

});

teardown(() => {
delete process.env.FIREBASE_TOKEN;

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

Restore the original FIREBASE_TOKEN instead of unconditionally deleting it, to avoid breaking other tests that might run in the same process.

      if ((globalThis as any).originalFirebaseToken === undefined) {
        delete process.env.FIREBASE_TOKEN;
      } else {
        process.env.FIREBASE_TOKEN = (globalThis as any).originalFirebaseToken;
      }

@joehan joehan force-pushed the refactor/migrate-to-native-fetch-part2 branch from fd88b59 to 7b512c3 Compare June 25, 2026 22:47
@joehan joehan force-pushed the refactor/migrate-to-native-fetch-part3 branch from 846f7b1 to 0624e98 Compare June 25, 2026 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants