[Unified Recorder] Call `proxy-tool` through dev-tool #18322

HarshaNalluru · 2021-10-22T20:28:55Z

Issue #17042

TODO

Future PRs

Allow providing docker run options along with the mocha and karma options for the test commands
Call mocha programmatically
- experiment with using dev-tool's own register instead of ts-node/register. That way we can really centralize the concerns in our own config/scripts.
Call karma programatically
@witemple-msft's comment
- All of this is nitpicky, but it's best to be explicit about the mutability of the volume. If we are running in playback mode, we should bind the volume read-only. In record mode, we should bind it read-write. On Linux hosts that have SELInux (Red Hat, Fedora), we'll also need the Z flag to relabel the volume. We can test for SELinux using the selinuxenabled command, and that'll exit 0 if it's enabled on the machine:
@scbedd raise a couple of interesting points
- docker run .. with concurrently stops gracefully when the tests end in Linux and Mac but not on windows, though the process(concurrently) ends. Figure out a way to properly stop the docker run .. in windows.

… harshan/issue/17042

check-enforcer · 2021-10-26T18:31:31Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

… harshan/issue/17042

witemple-msft

Overall I think this is pretty clever. The idea to just hit the /info endpoint of the proxy tool and start it when running the tests if it's not already running seems like it's going to be a nice developer experience.

I have a few broad questions:

What do we tell developers in error messages if they don't have docker?
Is there any setup required on the local machine for this to work?

Other than that, I have some feedback from the dev-tool perspective and some changes that we will need to make.

common/tools/dev-tool/src/commands/index.ts

common/tools/dev-tool/src/commands/testNode.ts

common/tools/dev-tool/src/util/testProxyUtils.ts

witemple-msft · 2021-10-27T15:51:51Z

common/tools/dev-tool/src/util/testProxyUtils.ts

+import { spawn } from "child_process";
+import fs from "fs";
+import path from "path";
+import { IncomingMessage, request, RequestOptions } from "http";


I'm in favor of using a simpler HTTP client in dev-tool such as axios.

This is the only HTTP request that is made in the whole of dev-tool.
I don't want to take a dependency on another package if this serves my purpose.

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").

core-rest-pipelines should help 🤔 at least in the future

That'll add circular dependencies @sadasant.
core packages depend on dev-tool

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.

That'll add circular dependencies @sadasant.

Not if you pin a version 🤔 since dev-tools is only a devDependency. But I agree with Will 👍

common/tools/dev-tool/src/commands/testNode.ts

…u/azure-sdk-for-js into harshan/issue/17042

… harshan/issue/17042

sdk/test-utils/recorder-new/test/testProxyTests.spec.ts

HarshaNalluru · 2021-11-17T01:15:51Z

eng/common/testproxy/test-proxy-tool.yml

-        -ArgumentList "--storage-location '${{ parameters.rootFolder }}'" `
+        -ArgumentList "--storage-location ${{ parameters.rootFolder }}" `
        -NoNewWindow -PassThru -RedirectStandardOutput $(Build.SourcesDirectory)/test-proxy.log
    displayName: 'Run the testproxy - windows'


@scbedd for reference

Analyze step will be fixed once this fix goes into the tools repo. Azure/azure-sdk-tools#2290

…u/azure-sdk-for-js into harshan/issue/17042

common/tools/dev-tool/src/util/checkWithTimeout.ts

sadasant

Looks really clean 🙂

jeremymeng

Looking forward to enjoying all the benefits that proxy-tool brings us!

jeremymeng · 2021-11-18T17:37:54Z

common/tools/dev-tool/src/commands/run/index.ts

+
+import { subCommand, makeCommandInfo } from "../../framework/command";
+
+export const commandInfo = makeCommandInfo("run", "run scripts such as test:node");


nit: test:node not accurate if the sub commands include both node and browser?

jeremymeng · 2021-11-18T17:41:47Z

common/tools/dev-tool/src/commands/run/testNodeJSInput.ts

+
+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}`,


do we still have packages that bundle the nodejs tests? I hope -r esm still work with those if any exists

So, three ways of running the packages

js rollup bundle

js dist-esm files

ts sources

I'll check if test:node-js-input is working for both 1st and 2nd.

HarshaNalluru · 2021-11-19T23:14:43Z

/azp run js - test-utils - ci

azure-pipelines · 2021-11-19T23:15:00Z

Azure Pipelines successfully started running 1 pipeline(s).

HarshaNalluru added 4 commits October 20, 2021 20:26

test-proxy starter code for starting

15ed59e

adding in the requirements

f5a9992

test-proxy starter code for starting

946bf0b

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

29ad0f3

… harshan/issue/17042

ghost added the dev-tool Issues related to the Azure SDK for JS dev-tool label Oct 22, 2021

HarshaNalluru added 6 commits October 22, 2021 20:30

clean up

4bbc37e

gets RootLocation

fc25043

os.platform() === "win32" check

4f0f2e6

cleanup

09a20ea

testProxyUtils.ts

e69fa22

checkpoint

be70341

HarshaNalluru added 7 commits October 26, 2021 20:17

node side looks like it's working ✔️

0938dc7

readme formatting

950f46b

same console.log in win and lin

fcd64ff

test:node-with-proxy

074bfe4

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

5f407f9

… harshan/issue/17042

"sdk-type": "utility",

ce99f54

lock file

cb3342d

witemple-msft requested changes Oct 27, 2021

View reviewed changes

HarshaNalluru added 10 commits October 27, 2021 20:59

"sdk-type": "utility",

4285651

Merge branch 'harshan/issue/18401' of https://github.com/HarshaNallur…

4adb560

…u/azure-sdk-for-js into harshan/issue/17042

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

acd56a9

… harshan/issue/17042

lock file

ddd3b7b

dev-tool test:browser

a7147c2

dotenv.config() call not needed since dev-tool does it by default

da195a1

run subcommand

378a342

Partly switching to "fs-extra"

803a205

fsExtra -> fs

91f8a53

test:node-{js|ts}-input

1c46de2

HarshaNalluru added 4 commits November 16, 2021 23:28

lock file

6713590

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

cb6c383

… harshan/issue/17042

dump logs

35a39a3

fix windows path

37d151d

HarshaNalluru commented Nov 17, 2021

View reviewed changes

sdk/test-utils/recorder-new/test/testProxyTests.spec.ts Show resolved Hide resolved

HarshaNalluru commented Nov 17, 2021

View reviewed changes

HarshaNalluru added 6 commits November 17, 2021 01:57

PROXY_MANUAL_START in karma.conf

e5d4ff0

duplication

a192f99

clean karma conf

5258da3

clean package.json

a76fcc2

waits for the proxy tool - draft

83f9d37

wait-for-proxy-endpoint finish

f124e79

This was referenced Nov 17, 2021

[Unified Recorder] Only run tests after the proxy tool is active #18712

Closed

Add proxy tool logs dump in the CI #18714

Closed

HarshaNalluru added 5 commits November 17, 2021 09:49

Update sdk/test-utils/recorder-new/test/testProxyTests.spec.ts

2283626

beautify the tests

b790604

Merge branch 'harshan/issue/17042' of https://github.com/HarshaNallur…

9743b8f

…u/azure-sdk-for-js into harshan/issue/17042

fix the test mode log

213a315

minor updates to tests

99300d4

sadasant reviewed Nov 18, 2021

View reviewed changes

common/tools/dev-tool/src/util/checkWithTimeout.ts Show resolved Hide resolved

sadasant approved these changes Nov 18, 2021

View reviewed changes

jeremymeng approved these changes Nov 18, 2021

View reviewed changes

HarshaNalluru added 2 commits November 19, 2021 00:06

test-info

227e21c

no need to pass test mode

7696173

HarshaNalluru merged commit 2de7b65 into Azure:main Nov 20, 2021

HarshaNalluru deleted the harshan/issue/17042 branch November 20, 2021 09:12

HarshaNalluru mentioned this pull request Nov 23, 2021

[Unified Recorder] Recorder should be able to start and stop the proxy tool #17042

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Unified Recorder] Call `proxy-tool` through dev-tool #18322

[Unified Recorder] Call `proxy-tool` through dev-tool #18322

HarshaNalluru commented Oct 22, 2021 •

edited

Loading

check-enforcer bot commented Oct 26, 2021

witemple-msft left a comment

witemple-msft Oct 27, 2021

HarshaNalluru Oct 28, 2021

witemple-msft Nov 8, 2021

sadasant Nov 8, 2021

HarshaNalluru Nov 8, 2021

witemple-msft Nov 8, 2021

sadasant Nov 8, 2021

HarshaNalluru Nov 17, 2021

HarshaNalluru Nov 17, 2021

sadasant left a comment

jeremymeng left a comment

jeremymeng Nov 18, 2021

jeremymeng Nov 18, 2021

HarshaNalluru Nov 18, 2021

HarshaNalluru commented Nov 19, 2021

azure-pipelines bot commented Nov 19, 2021


		import { subCommand, makeCommandInfo } from "../../framework/command";

		export const commandInfo = makeCommandInfo("run", "run scripts such as test:node");

[Unified Recorder] Call proxy-tool through dev-tool #18322

[Unified Recorder] Call proxy-tool through dev-tool #18322

Conversation

HarshaNalluru commented Oct 22, 2021 • edited Loading

Issue #17042

TODO

Future PRs

check-enforcer bot commented Oct 26, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

witemple-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

jeremymeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru commented Nov 19, 2021

azure-pipelines bot commented Nov 19, 2021

[Unified Recorder] Call `proxy-tool` through dev-tool #18322

[Unified Recorder] Call `proxy-tool` through dev-tool #18322

HarshaNalluru commented Oct 22, 2021 •

edited

Loading