-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Unified Recorder] Call proxy-tool
through dev-tool
#18322
Changes from 78 commits
15ed59e
f5a9992
946bf0b
29ad0f3
4bbc37e
fc25043
4f0f2e6
09a20ea
e69fa22
be70341
0938dc7
950f46b
fcd64ff
074bfe4
5f407f9
ce99f54
cb3342d
4285651
4adb560
acd56a9
ddd3b7b
a7147c2
da195a1
378a342
803a205
91f8a53
1c46de2
4469c36
d204966
b986e5f
0be5730
25968e7
bc48123
f2e1fbf
b0cda6a
77e8229
660ae89
a6d1dea
63cdb75
94e4e0d
6ce7ec8
f6a62c6
41fcb8d
dd0161c
7af4f16
210ed4d
bed8368
79ddd0f
ad06342
2f1ab3e
3817355
c3e44cb
788a308
e4f1ed0
881b3fc
baeea7d
fce7d97
c40196d
80df60b
58fc8d8
b074e27
348e1c1
8dddf51
6713590
cb6c383
35a39a3
37d151d
e5d4ff0
a192f99
5258da3
a76fcc2
83f9d37
f124e79
2283626
b790604
9743b8f
213a315
99300d4
227e21c
7696173
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license | ||
|
||
import { subCommand, makeCommandInfo } from "../../framework/command"; | ||
|
||
export const commandInfo = makeCommandInfo("run", "run scripts such as test:node"); | ||
|
||
export default subCommand(commandInfo, { | ||
"test:node-ts-input": () => import("./testNodeTSInput"), | ||
"test:node-js-input": () => import("./testNodeJSInput"), | ||
"test:browser": () => import("./testBrowser") | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license | ||
|
||
import { leafCommand, makeCommandInfo } from "../../framework/command"; | ||
import { runTestsWithProxyTool } from "../../util/testUtils"; | ||
|
||
export const commandInfo = makeCommandInfo( | ||
"test:browser", | ||
"runs the browser tests using karma with the default and the provided options; starts the proxy-tool in record and playback modes", | ||
{ | ||
karma: { | ||
kind: "string", | ||
description: "Karma options (such as --single-run)", | ||
default: "--single-run" | ||
} | ||
} | ||
); | ||
|
||
export default leafCommand(commandInfo, async (options) => { | ||
return runTestsWithProxyTool({ | ||
command: `karma start ${options.karma}`, | ||
name: "browser-tests" | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license | ||
|
||
import { leafCommand, makeCommandInfo } from "../../framework/command"; | ||
import { runTestsWithProxyTool } from "../../util/testUtils"; | ||
|
||
export const commandInfo = makeCommandInfo( | ||
"test:node-js-input", | ||
"runs the node tests using mocha with the default and the provided options; starts the proxy-tool in record and playback modes", | ||
{ | ||
mocha: { | ||
kind: "string", | ||
description: | ||
"Mocha options along with the bundled test file(JS) with rollup as expected by mocha", | ||
default: '--timeout 5000000 "dist-esm/test/{,!(browser)/**/}/*.spec.js"' | ||
} | ||
} | ||
); | ||
|
||
export default leafCommand(commandInfo, async (options) => { | ||
return runTestsWithProxyTool({ | ||
command: `nyc mocha -r esm --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --full-trace ${options.mocha}`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we still have packages that bundle the nodejs tests? I hope There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, three ways of running the packages
I'll check if |
||
name: "node-tests" | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license | ||
|
||
import { leafCommand, makeCommandInfo } from "../../framework/command"; | ||
import { runTestsWithProxyTool } from "../../util/testUtils"; | ||
|
||
export const commandInfo = makeCommandInfo( | ||
"test:node-ts-input", | ||
"runs the node tests using mocha with the default and the provided options; starts the proxy-tool in record and playback modes", | ||
{ | ||
mocha: { | ||
kind: "string", | ||
description: | ||
"Mocha options along with the test files(glob pattern) in TS as expected by mocha", | ||
default: '--timeout 1200000 --exclude "test/**/browser/*.spec.ts" "test/**/*.spec.ts"' | ||
} | ||
} | ||
); | ||
|
||
export default leafCommand(commandInfo, async (options) => { | ||
return runTestsWithProxyTool({ | ||
command: `mocha -r esm -r ts-node/register --reporter ../../../common/tools/mocha-multi-reporter.js --full-trace ${options.mocha}`, | ||
name: "node-tests" | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license | ||
|
||
import { subCommand, makeCommandInfo } from "../../framework/command"; | ||
|
||
export const commandInfo = makeCommandInfo( | ||
"test-proxy", | ||
"runs the proxy-tool with the `docker run ...` command" | ||
); | ||
|
||
export default subCommand(commandInfo, { | ||
start: () => import("./start"), | ||
"wait-for-proxy-endpoint": () => import("./waitForProxyEndpoint") | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { leafCommand, makeCommandInfo } from "../../framework/command"; | ||
import { config } from "dotenv"; | ||
import { startProxyTool } from "../../util/testProxyUtils"; | ||
config(); | ||
|
||
export const commandInfo = makeCommandInfo( | ||
"test-proxy", | ||
"runs the proxy-tool with the `docker run ...` command", | ||
{} | ||
); | ||
|
||
export default leafCommand(commandInfo, async (_options) => { | ||
await startProxyTool(process.env.TEST_MODE); | ||
return true; | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { leafCommand, makeCommandInfo } from "../../framework/command"; | ||
import { config } from "dotenv"; | ||
import { isProxyToolActive } from "../../util/testProxyUtils"; | ||
import { checkWithTimeout } from "../../util/checkWithTimeout"; | ||
config(); | ||
|
||
export const commandInfo = makeCommandInfo( | ||
"test-proxy", | ||
"waits for the proxy tool to be active or fails in 2 minutes", | ||
{} | ||
); | ||
|
||
export default leafCommand(commandInfo, async (_options) => { | ||
const result = await checkWithTimeout(isProxyToolActive, 1000, 120000); | ||
return result; | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license | ||
|
||
import { createPrinter } from "./printer"; | ||
const log = createPrinter("check-with-timeout"); | ||
|
||
/** | ||
* - Maximum wait duration for the expected event to happen = `10000 ms`(default value is 10 seconds)(= maxWaitTimeInMilliseconds) | ||
* - Keep checking whether the predicate is true after every `1000 ms`(default value is 1 second) (= delayBetweenRetriesInMilliseconds) | ||
*/ | ||
export async function checkWithTimeout( | ||
HarshaNalluru marked this conversation as resolved.
Show resolved
Hide resolved
|
||
predicate: () => boolean | Promise<boolean>, | ||
delayBetweenRetriesInMilliseconds: number = 1000, | ||
maxWaitTimeInMilliseconds: number = 10000 | ||
): Promise<boolean> { | ||
const maxTime = Date.now() + maxWaitTimeInMilliseconds; | ||
while (Date.now() < maxTime) { | ||
if (await predicate()) { | ||
log.info(`checkWithTimeout condition returned true`); | ||
return true; | ||
} | ||
await delay(delayBetweenRetriesInMilliseconds); | ||
} | ||
return false; | ||
} | ||
|
||
async function delay(timeInMs: number) { | ||
return new Promise((resolve) => { | ||
log.info(`waiting for ${timeInMs}ms`); | ||
setTimeout(resolve, timeInMs); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { spawn } from "child_process"; | ||
import path from "path"; | ||
import { IncomingMessage, request, RequestOptions } from "http"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm in favor of using a simpler HTTP client in dev-tool such as axios. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only HTTP request that is made in the whole of dev-tool. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very well, When/if we have more HTTP usage I'd like to replace it with axios, node-fetch, etc. (something a little bit more "batteries-included"). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. core-rest-pipelines should help 🤔 at least in the future There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That'll add circular dependencies @sadasant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we want to use core for incidental stuff in dev-tool. It's designed for the client libraries and unless we really have some kind of request pipeline in dev-tool where performance and consistent configurability with the clients is important, I think it'll be nicer to just use a simple API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not if you pin a version 🤔 since dev-tools is only a devDependency. But I agree with Will 👍 |
||
import fs from "fs-extra"; | ||
import { createPrinter } from "./printer"; | ||
|
||
const log = createPrinter("test-proxy"); | ||
export async function startProxyTool(mode: string | undefined) { | ||
log.info(`===TEST_MODE="${mode}"===`); | ||
log.info( | ||
`Attempting to start test proxy at http://localhost:5000 & https://localhost:5001.\n` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can these ports be parametrized? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logged an issue for this at #15829, will tackle in a future PR. |
||
); | ||
|
||
const subprocess = spawn(await getDockerRunCommand(), [], { | ||
shell: true | ||
}); | ||
|
||
const outFileName = "test-proxy-output.log"; | ||
const out = fs.createWriteStream(`./${outFileName}`, { flags: 'a' }); | ||
subprocess.stdout.pipe(out); | ||
subprocess.stderr.pipe(out); | ||
|
||
log.info(`Check the output file "${outFileName}" for test-proxy logs.`); | ||
} | ||
|
||
async function getRootLocation(start?: string): Promise<string> { | ||
start ??= process.cwd(); | ||
if (await fs.pathExists(path.join(start, "rush.json"))) { | ||
return start; | ||
} else { | ||
const nextPath = path.resolve(start, ".."); | ||
if (nextPath === start) { | ||
throw new Error("Reached filesystem root, but no rush.json was found."); | ||
} else { | ||
return getRootLocation(nextPath); | ||
} | ||
} | ||
} | ||
|
||
async function getDockerRunCommand() { | ||
const repoRoot = await getRootLocation(); // /workspaces/azure-sdk-for-js/ | ||
const testProxyRecordingsLocation = "/etc/testproxy"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @HarshaNalluru the PR is submitted to sync the proxy tool tag update, I will need to change this script as well. That will keep everything in sync. |
||
const allowLocalhostAccess = "--add-host host.docker.internal:host-gateway"; | ||
const imageToLoad = `azsdkengsys.azurecr.io/engsys/testproxy-lin:${await getImageTag()}`; | ||
return `docker run -v ${repoRoot}:${testProxyRecordingsLocation} -p 5001:5001 -p 5000:5000 ${allowLocalhostAccess} ${imageToLoad}`; | ||
HarshaNalluru marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can these ports be parametrized? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logged an issue for this at #15829, will tackle in a future PR. |
||
} | ||
|
||
export async function isProxyToolActive() { | ||
try { | ||
await makeRequest("http://localhost:5000/info/available", {}); | ||
log.info(`Proxy tool seems to be active at http://localhost:5000\n`); | ||
return true; | ||
} catch (error) { | ||
return false; | ||
} | ||
} | ||
|
||
async function makeRequest(uri: string, requestOptions: RequestOptions): Promise<IncomingMessage> { | ||
return new Promise<IncomingMessage>((resolve, reject) => { | ||
const req = request(uri, requestOptions, resolve); | ||
req.once("error", reject); | ||
req.end(); | ||
}); | ||
} | ||
|
||
async function getImageTag() { | ||
// Grab the tag from the `/eng/common/testproxy/docker-start-proxy.ps1` file [..is used to run the proxy-tool in the CI] | ||
// | ||
// $SELECTED_IMAGE_TAG = "1147815"; | ||
// (Bot regularly updates the tag in the file above.) | ||
try { | ||
const contentInPWSHScript = await fs.readFile( | ||
`${path.join(await getRootLocation(), "eng/common/testproxy/docker-start-proxy.ps1")}`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this path be parametrized? In case someone wants to change it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would someone not want the root location? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This path is to find the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh ok ok. Gotcha! Thanks! |
||
"utf-8" | ||
); | ||
const tag = contentInPWSHScript.match(/\$SELECTED_IMAGE_TAG \= \"(.*)\"/)![1]; | ||
log.info(`Image tag obtained from the powershell script => ${tag}\n`); | ||
return tag; | ||
} catch (_) { | ||
log.warn( | ||
`Unable to get the image tag from the powershell script, trying "latest" tag instead\n` | ||
); | ||
return "latest"; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import { isProxyToolActive } from "./testProxyUtils"; | ||
import concurrently from "concurrently"; | ||
import { createPrinter } from "./printer"; | ||
|
||
const log = createPrinter("preparing-proxy-tool"); | ||
|
||
async function shouldRunProxyTool(): Promise<boolean> { | ||
const mode = process.env.TEST_MODE; | ||
if (mode === "live") { | ||
return false; // No need to start the proxy tool in the live mode | ||
} else { | ||
const isActive = await isProxyToolActive(); | ||
if (isActive) { | ||
// No need to run a new one if it is already active | ||
// Especially, CI uses this path | ||
log.info( | ||
`Proxy tool seems to be active, not attempting to start the test proxy at http://localhost:5000 & https://localhost:5001.\n` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same parametrized ports question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logged an issue for this at #15829, will tackle in a future PR. |
||
); | ||
} | ||
return !isActive; | ||
} | ||
} | ||
|
||
export async function runTestsWithProxyTool(testCommandObj: concurrently.CommandObj) { | ||
if ( | ||
await shouldRunProxyTool() // Boolean to figure out if we need to run just the mocha command or the test-proxy too | ||
) { | ||
const testProxyCMD = "dev-tool test-proxy start"; | ||
const waitForProxyEndpointCMD = "dev-tool test-proxy wait-for-proxy-endpoint"; | ||
await concurrently( | ||
[ | ||
{ command: testProxyCMD }, | ||
{ | ||
command: `${waitForProxyEndpointCMD} && ${testCommandObj.command}`, // Waits for the proxy endpoint to be active and then starts running the tests | ||
name: testCommandObj.name | ||
} | ||
], | ||
{ | ||
killOthers: ["failure", "success"], | ||
successCondition: "first" | ||
} | ||
); | ||
} else { | ||
await concurrently([testCommandObj]); | ||
} | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
test:node
not accurate if the sub commands include both node and browser?