From 1f24bd19e27f600629db683a389a95825aad2517 Mon Sep 17 00:00:00 2001 From: Timo van Veenendaal Date: Tue, 7 Feb 2023 16:52:10 -0800 Subject: [PATCH 1/3] [recorder] Figure out assets.json from RELATIVE_RECORDINGS_PATH --- sdk/test-utils/recorder/karma.conf.js | 3 +- sdk/test-utils/recorder/src/index.ts | 2 +- sdk/test-utils/recorder/src/recorder.ts | 36 ++++++++++++++++--- .../utils/relativePathCalculator.browser.ts | 27 -------------- .../src/utils/relativePathCalculator.ts | 27 -------------- .../recorder/src/utils/sessionFilePath.ts | 9 +++++ 6 files changed, 43 insertions(+), 61 deletions(-) diff --git a/sdk/test-utils/recorder/karma.conf.js b/sdk/test-utils/recorder/karma.conf.js index 0f972697940e..fcd4f606de28 100644 --- a/sdk/test-utils/recorder/karma.conf.js +++ b/sdk/test-utils/recorder/karma.conf.js @@ -4,7 +4,6 @@ process.env.CHROME_BIN = require("puppeteer").executablePath(); require("dotenv").config({ path: "../.env" }); process.env.RECORDINGS_RELATIVE_PATH = relativeRecordingsPath(); -process.env.RECORDING_ASSETS_PATH = relativeAssetsPath(); module.exports = function (config) { config.set({ @@ -50,7 +49,7 @@ module.exports = function (config) { // inject following environment values into browser testing with window.__env__ // environment values MUST be exported or set with same console running "karma start" // https://www.npmjs.com/package/karma-env-preprocessor - envPreprocessor: ["RECORDINGS_RELATIVE_PATH", "PROXY_MANUAL_START", "RECORDING_ASSETS_PATH"], + envPreprocessor: ["RECORDINGS_RELATIVE_PATH", "PROXY_MANUAL_START"], // test results reporter to use // possible values: 'dots', 'progress' diff --git a/sdk/test-utils/recorder/src/index.ts b/sdk/test-utils/recorder/src/index.ts index 4831adc13db2..c4add19c6c5b 100644 --- a/sdk/test-utils/recorder/src/index.ts +++ b/sdk/test-utils/recorder/src/index.ts @@ -2,7 +2,7 @@ // Licensed under the MIT license. export { Recorder } from "./recorder"; -export { relativeRecordingsPath, relativeAssetsPath } from "./utils/relativePathCalculator"; +export { relativeRecordingsPath } from "./utils/relativePathCalculator"; export { SanitizerOptions, RecorderStartOptions, diff --git a/sdk/test-utils/recorder/src/recorder.ts b/sdk/test-utils/recorder/src/recorder.ts index c6af718ef78e..7001bb8394a1 100644 --- a/sdk/test-utils/recorder/src/recorder.ts +++ b/sdk/test-utils/recorder/src/recorder.ts @@ -21,7 +21,7 @@ import { RecordingStateManager, } from "./utils/utils"; import { Test } from "mocha"; -import { sessionFilePath } from "./utils/sessionFilePath"; +import { assetsJsonPath, sessionFilePath } from "./utils/sessionFilePath"; import { SanitizerOptions } from "./utils/utils"; import { paths } from "./utils/paths"; import { addSanitizers, transformsInfo } from "./sanitizer"; @@ -43,7 +43,6 @@ import { isNode } from "@azure/core-util"; import { env } from "./utils/env"; import { decodeBase64 } from "./utils/encoding"; import { isHttpHeadersLike } from "./utils/typeGuards"; -import { relativeAssetsPath } from "./utils/relativePathCalculator"; /** * This client manages the recorder life cycle and interacts with the proxy-tool to do the recording, @@ -70,7 +69,7 @@ export class Recorder { if (isRecordMode() || isPlaybackMode()) { if (this.testContext) { this.sessionFile = sessionFilePath(this.testContext); - this.assetsJson = relativeAssetsPath(); + this.assetsJson = assetsJsonPath(); logger.info(`[Recorder#constructor] Using a session file located at ${this.sessionFile}`); this.httpClient = createDefaultHttpClient(); @@ -260,10 +259,39 @@ export class Recorder { logger.verbose("[Recorder#start] Setting redirect mode"); await setRecordingOptions(Recorder.url, this.httpClient, { handleRedirects: !isNode }); logger.verbose("[Recorder#start] Sending the start request to the test proxy"); - const rsp = await this.httpClient.sendRequest({ + let rsp = await this.httpClient.sendRequest({ ...req, allowInsecureConnection: true, }); + + // If the error is due to the assets.json not existing, try again without specifying an assets.json. This will + // occur with SDKs that have not migrated to asset sync yet. + // TODO: remove once everyone has migrated to asset sync + if (rsp.status === 400 && rsp.headers.get("x-request-known-exception") === "true") { + const errorMessage = decodeBase64(rsp.headers.get("x-request-known-exception-error")!); + if ( + errorMessage.includes("The provided assets") && + errorMessage.includes("does not exist") + ) { + logger.info( + "[Recorder#start] start request failed, trying again without assets.json specified" + ); + + const retryRequest = createRecordingRequest( + startUri, + this.sessionFile, + this.recordingId, + "POST", + undefined + ); + + rsp = await this.httpClient.sendRequest({ + ...retryRequest, + allowInsecureConnection: true, + }); + } + } + if (rsp.status !== 200) { logger.error("[Recorder#start] Could not start the recorder", rsp); throw new RecorderError("Start request failed."); diff --git a/sdk/test-utils/recorder/src/utils/relativePathCalculator.browser.ts b/sdk/test-utils/recorder/src/utils/relativePathCalculator.browser.ts index 3a4edbe47056..bb4ac340a7a3 100644 --- a/sdk/test-utils/recorder/src/utils/relativePathCalculator.browser.ts +++ b/sdk/test-utils/recorder/src/utils/relativePathCalculator.browser.ts @@ -13,30 +13,3 @@ export function relativeRecordingsPath(): string { ); } } - -/** - * ONLY WORKS IN THE NODE.JS ENVIRONMENT - * - * Returns the potential assets.json for the project using `process.cwd()`. - * - * Note for browser tests: - * 1. Supposed to be called from karma.conf.js in the package for which the testing is being done. - * 2. Set this `RECORDING_ASSETS_PATH` as an env variable - * ```js - * const { relativeRecordingsPathForBrowser } = require("@azure-tools/test-recorder-new"); - * process.env.RECORDING_ASSETS_PATH = relativeRecordingsPathForBrowser(); - * ``` - * 3. Add "RECORDING_ASSETS_PATH" in the `envPreprocessor` array to let this be loaded in the browser environment. - * ``` - * envPreprocessor: ["RECORDING_ASSETS_PATH"], - * ``` - * - * `RECORDING_ASSETS_PATH` in the browser environment is used in the recorder to tell the proxy-tool about whether or not to pass additional body argument - * `x-recording-assets-file` to playback|record/Start. Doing so enables the proxy to auto-restore files from a remote location. - * - * @export - * @returns {string} location of the relative path to discovered assets.json - `sdk/storage/storage-blob/assets.json` for example. - */ -export function relativeAssetsPath(): string | undefined { - return env.RECORDING_ASSETS_PATH; -} diff --git a/sdk/test-utils/recorder/src/utils/relativePathCalculator.ts b/sdk/test-utils/recorder/src/utils/relativePathCalculator.ts index 0283e7cb05d9..114a5fc16e72 100644 --- a/sdk/test-utils/recorder/src/utils/relativePathCalculator.ts +++ b/sdk/test-utils/recorder/src/utils/relativePathCalculator.ts @@ -76,30 +76,3 @@ function relativePackagePath() { export function relativeRecordingsPath(): string { return toSafePath(path.join(relativePackagePath(), "recordings")); } - -/** - * Returns the potential assets.json for the project using `process.cwd()`. - * - * Note for browser tests: - * 1. Supposed to be called from karma.conf.js in the package for which the testing is being done. - * 2. Set this `RECORDING_ASSETS_PATH` as an env variable - * ```js - * const { relativeRecordingsPathForBrowser } = require("@azure-tools/test-recorder-new"); - * process.env.RECORDING_ASSETS_PATH = relativeRecordingsPathForBrowser(); - * ``` - * 3. Add "RECORDING_ASSETS_PATH" in the `envPreprocessor` array to let this be loaded in the browser environment. - * ``` - * envPreprocessor: ["RECORDING_ASSETS_PATH"], - * ``` - * - * `RECORDING_ASSETS_PATH` in the browser environment is used in the recorder to tell the proxy-tool about whether or not to pass additional body argument - * `x-recording-assets-file` to playback|record/Start. Doing so enables the proxy to auto-restore files from a remote location. - * - * @export - * @returns {string} location of the relative path to discovered assets.json - `sdk/storage/storage-blob/assets.json` for example, or undefined if the path does not exist - */ -export function relativeAssetsPath(): string | undefined { - return fs.existsSync("assets.json") - ? toSafePath(path.join(relativePackagePath(), "assets.json")) - : undefined; -} diff --git a/sdk/test-utils/recorder/src/utils/sessionFilePath.ts b/sdk/test-utils/recorder/src/utils/sessionFilePath.ts index 8db5e38bca5e..b70199888c85 100644 --- a/sdk/test-utils/recorder/src/utils/sessionFilePath.ts +++ b/sdk/test-utils/recorder/src/utils/sessionFilePath.ts @@ -29,3 +29,12 @@ export function recordingFilePath(testContext: Mocha.Test): string { testContext.title ); } + +export function assetsJsonPath(): string { + // Hacky solution using substring works around the fact that: + // 1) the relativeRecordingsPath may not exist on disk (so relativeRecordingsPath()/../assets.json might not exist either, can't use ..) + // 2) `path` (and therefore `path.dirname`) is not available in the browser. + const recordingsPath = relativeRecordingsPath(); + const sdkDir = recordingsPath.substring(0, recordingsPath.lastIndexOf("/")); + return `${sdkDir}/assets.json`; +} From 315b311f1b6635b837a16a95778aa57496607c20 Mon Sep 17 00:00:00 2001 From: Timo van Veenendaal Date: Wed, 8 Feb 2023 10:50:30 -0800 Subject: [PATCH 2/3] Drop PROXY_MANUAL_START from Karma configuration --- sdk/test-utils/recorder/karma.conf.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test-utils/recorder/karma.conf.js b/sdk/test-utils/recorder/karma.conf.js index fcd4f606de28..157bf23b27ab 100644 --- a/sdk/test-utils/recorder/karma.conf.js +++ b/sdk/test-utils/recorder/karma.conf.js @@ -49,7 +49,7 @@ module.exports = function (config) { // inject following environment values into browser testing with window.__env__ // environment values MUST be exported or set with same console running "karma start" // https://www.npmjs.com/package/karma-env-preprocessor - envPreprocessor: ["RECORDINGS_RELATIVE_PATH", "PROXY_MANUAL_START"], + envPreprocessor: ["RECORDINGS_RELATIVE_PATH"], // test results reporter to use // possible values: 'dots', 'progress' From e5a70c6e5b7c65b0fa0a33762a4baac1c8ffeeb6 Mon Sep 17 00:00:00 2001 From: Timo van Veenendaal Date: Wed, 8 Feb 2023 10:47:13 -0800 Subject: [PATCH 3/3] CHANGELOG entry --- sdk/test-utils/recorder/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/test-utils/recorder/CHANGELOG.md b/sdk/test-utils/recorder/CHANGELOG.md index 7f9532b93d9a..b28bb5304361 100644 --- a/sdk/test-utils/recorder/CHANGELOG.md +++ b/sdk/test-utils/recorder/CHANGELOG.md @@ -9,6 +9,7 @@ - Allow mapping the test-proxy tool to ports other than just 5000(for HTTP) using the environment variable `TEST_PROXY_HTTP_PORT`(and `TEST_PROXY_HTTPS_PORT` for 5001(for HTTPS)). - If `TEST_PROXY_HTTP_PORT` is undefined, we'll try for 5000 as usual. - For browsers, this variable has to be added as part of the environment variables listed under `envPreprocessor` array in `karma.conf.js` so that the recorder knows the port to hit. +- Added support for the asset sync tool. If an `assets.json` exists in the package directory, the recorder will fetch recordings from the external repo. ### Breaking Changes