fix(eventarc): resolve the emulator host when the client is constructed - #3223
fix(eventarc): resolve the emulator host when the client is constructed#3223kyungseopk1m wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the EventarcApiClient to resolve the Eventarc emulator host at construction time instead of at request time, and adds unit tests to verify this behavior. The feedback recommends saving and restoring the original value of process.env.CLOUD_EVENTARC_EMULATOR_HOST in the test hooks to prevent test pollution and potential flakiness.
| let mockAccessToken: string; | ||
| let httpStub: sinon.SinonStub; | ||
| let accessTokenStub: sinon.SinonStub; | ||
|
|
||
| before(() => { | ||
| mockAccessToken = utils.generateRandomAccessToken(); | ||
| accessTokenStub = utils.stubGetAccessToken(mockAccessToken); | ||
| }); | ||
|
|
||
| after(() => { | ||
| accessTokenStub?.restore(); | ||
| }); |
There was a problem hiding this comment.
Modifying process.env in tests without restoring its original value can lead to test pollution and flaky tests if other test suites run in the same process. It is highly recommended to capture the original value of CLOUD_EVENTARC_EMULATOR_HOST in the before hook and restore it in the after hook.
let mockAccessToken: string;
let httpStub: sinon.SinonStub;
let accessTokenStub: sinon.SinonStub;
let originalEmulatorHost: string | undefined;
before(() => {
mockAccessToken = utils.generateRandomAccessToken();
accessTokenStub = utils.stubGetAccessToken(mockAccessToken);
originalEmulatorHost = process.env.CLOUD_EVENTARC_EMULATOR_HOST;
});
after(() => {
accessTokenStub?.restore();
if (originalEmulatorHost === undefined) {
delete process.env.CLOUD_EVENTARC_EMULATOR_HOST;
} else {
process.env.CLOUD_EVENTARC_EMULATOR_HOST = originalEmulatorHost;
}
});|
Done in 5e5a1db. |
Discussion
Follows the same fix as #3167 (issue #3059), which moved
CLOUD_TASKS_EMULATOR_HOSTinFunctionsApiClientto construction time.EventarcApiClientreadsprocess.env.CLOUD_EVENTARC_EMULATOR_HOSTon every publish rather than when the client is built, so the routing of an existing channel depends on whatever the environment happens to hold at publish time. Create a production client, set the variable, create an emulator client, then unset it, and both publish to production.This moves the lookup into the constructor and stores it on a readonly field, matching
FunctionsApiClientandAbstractAuthRequestHandler, which both capture their emulator host up front.Behavior change
Code that sets
CLOUD_EVENTARC_EMULATOR_HOSTafter obtaining a channel will now keep hitting production. Nothing in the SDK, its tests, or #1686 documents that ordering as supported, and Cloud Tasks and Auth already behave this way, but it is user visible so it is worth calling out.Scope
This keeps
??instead of the.trim() || undefinednormalization used inFunctionsApiClient. There the value is a truthiness flag, checked in four places, so a whitespace only value has to collapse toundefined. Here it is a plain URL prefix used once, and normalizing it would change what''and' 'do today, which is unrelated to the caching fix.Testing
Two tests in
test/unit/eventarc/eventarc.spec.ts, built the same way as the multi client test #3167 added. One covers the reported direction, an emulator channel created while the variable is set keeps using the emulator after it is cleared. The other covers the opposite direction, a production channel created before the variable is set keeps using production. Reverting the source fails both. The full unit suite passes at 6066.