[Unified Recorder] Support "variables" in recordings with proxy-tool #18585

HarshaNalluru · 2021-11-09T00:39:29Z

Issue #18580

What?

Allows storing the dynamically created variables in the record mode. Recorder registers the variables as part of the recording. Using the "variables" in playback mode would give the key-value pairs that are stored in record mode.

Example:

if (!isPlaybackMode()) {
  recorder.variables["random-1"] = `random-${Math.ceil(Math.random() * 1000 + 1000)}`;
}

Use this recorder.variables["random-1"] wherever you'd like to use in your test after the if-block.
(This would work in all three modes - record/playback/live just by adding the if-block above)

Internals(How does it work?):

recorder.stop() call sends the variables to the proxy-tool (in record mode) to save them in recordings
recorder.start() call loads those variables given by the proxy tool (in playback mode) that are picked up from the recordings

seankane-msft

LGTM, should probably have a JS specialist give the final approval

sadasant · 2021-11-10T00:45:46Z

sdk/test-utils/recorder-new/src/core-v2-recorder.ts

@@ -181,6 +203,10 @@ export class TestProxyHttpClient {
        const req = this._createRecordingRequest(stopUri);
        req.headers.set("x-recording-save", "true");

+        if (isRecordMode()) {
+          req.headers.set("Content-Type", "application/json");
+          req.body = JSON.stringify(variables ?? this.variables);


Wouldn’t this mean that we’re setting req.body to undefined if there are no variables? Is that desired?

this.variables is initialized as {}.

So, no problem.

sadasant

Looks good to me!

witemple-msft

It's clever and the implementation seems very simple. Since the variables are sent as JSON, I wonder why we can't store more than just strings. For example, I might like to store a number or even a nested object.

If you're interested in that idea, we can explore how the consumers of this API can strongly type that.

sdk/test-utils/recorder-new/CHANGELOG.md

sdk/test-utils/recorder-new/src/core-v2-recorder.ts

witemple-msft · 2021-11-10T17:07:56Z

sdk/test-utils/recorder-new/src/core-v2-recorder.ts

@@ -151,6 +170,9 @@ export class TestProxyHttpClient {
            throw new RecorderError("No recording ID returned for a successful start request.");
          }
          this.recordingId = id;
+          if (isPlaybackMode()) {
+            this.variables = !rsp.bodyAsText ? {} : JSON.parse(rsp.bodyAsText);


Suggested change

this.variables = !rsp.bodyAsText ? {} : JSON.parse(rsp.bodyAsText);

this.variables = JSON.parse(rsp.bodyAsText ?? "{}") as Record<string, string>;

Is the body not automatically parsed given the content type?

No, that happens in the deserializationPolicy

Haven't checked,

sdk/test-utils/recorder-new/src/core-v2-recorder.ts

jeremymeng · 2021-11-10T17:59:26Z

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

+        recorder.variables["random-2"] = "known-string";
+      }
+
+      await makeRequestAndVerifyResponse(


Could you explain what this is verifying? I see a variable is set, the variable is used in the request path, and test server seems always return "I am the answer!" but am not clear what's involved

oh, in playback mode, these variables will be available?

makeRequestAndVerifyResponse is a generic method and is applied for all the tests in this test suite.
It was initially created for the rest of the tests in the describe block.
All it does is make the request and check the response with what we are supposed to get.

All tests are run in the record, playback, and live modes in sequence in the same run.

The idea of the test is that the random string won't change in playback mode, and it'll reuse whatever has been created in the record mode.

Any suggestions on how I can make this clear? or improve the test?

This is fine. setting is in record mode but reading is in playback mode so I don't think there is a straight-forward way to test them as a whole. but end-to-end is working so we are good.

Co-authored-by: Will Temple <witemple@microsoft.com>

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

HarshaNalluru · 2021-11-11T21:27:27Z

@witemple-msft

It's clever and the implementation seems very simple. Since the variables are sent as JSON, I wonder why we can't store more than just strings. For example, I might like to store a number or even a nested object.

I did try to support numbers, but proxy-tool seems to be doing some validation on its end which is failing for numbers. So, kept strings only as of now. I provided the info to Scott. We'll see once the proxy tool's support improves.

If you're interested in that idea, we can explore how the consumers of this API can strongly type that.

I was thinking in those lines but as we only support strings at the moment, not much is needed at the moment.
I'm definitely interested 💗, let's recoup once we have more support.

Tracking your comments in the epic #15829 for now.

HarshaNalluru added 7 commits November 9, 2021 00:11

variables

1f229c6

remove console.ogs

54c775e

Add docs

db903be

changelog

fc5152a

recorder.variables

9443cd7

recordings

bf8b11a

more docs

ba0e5bf

HarshaNalluru requested a review from sadasant as a code owner November 9, 2021 00:39

HarshaNalluru requested review from mccoyp, scbedd, deyaaeldeen, seankane-msft, witemple-msft and jeremymeng November 9, 2021 00:41

seankane-msft reviewed Nov 9, 2021

View reviewed changes

HarshaNalluru mentioned this pull request Nov 9, 2021

[Unified Recorder] Features #15829

Closed

97 tasks

sadasant reviewed Nov 10, 2021

View reviewed changes

sadasant approved these changes Nov 10, 2021

View reviewed changes

witemple-msft approved these changes Nov 10, 2021

View reviewed changes

jeremymeng reviewed Nov 10, 2021

View reviewed changes

HarshaNalluru and others added 5 commits November 11, 2021 12:54

Update sdk/test-utils/recorder-new/CHANGELOG.md

d8dc9ad

Co-authored-by: Will Temple <witemple@microsoft.com>

Update sdk/test-utils/recorder-new/CHANGELOG.md

c9a9497

Co-authored-by: Will Temple <witemple@microsoft.com>

Update sdk/test-utils/recorder-new/src/core-v2-recorder.ts

802ebf1

Co-authored-by: Will Temple <witemple@microsoft.com>

feedback

87f1599

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

931590e

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

HarshaNalluru requested a review from jeremymeng November 11, 2021 21:09

jeremymeng approved these changes Nov 11, 2021

View reviewed changes

HarshaNalluru merged commit a5d9897 into Azure:main Nov 11, 2021

HarshaNalluru deleted the harshan/issue/18580 branch November 11, 2021 22:16

HarshaNalluru mentioned this pull request Nov 11, 2021

[Unified Recorder] Allow saving the randomly generated variable in record mode and give it back in playback. #18580

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Unified Recorder] Support "variables" in recordings with proxy-tool #18585

[Unified Recorder] Support "variables" in recordings with proxy-tool #18585

HarshaNalluru commented Nov 9, 2021 •

edited

Loading

seankane-msft left a comment

sadasant Nov 10, 2021

HarshaNalluru Nov 10, 2021

sadasant left a comment

witemple-msft left a comment

witemple-msft Nov 10, 2021

jeremymeng Nov 10, 2021

HarshaNalluru Nov 11, 2021

jeremymeng Nov 10, 2021

jeremymeng Nov 10, 2021

HarshaNalluru Nov 11, 2021

jeremymeng Nov 11, 2021

HarshaNalluru commented Nov 11, 2021

	this.variables = !rsp.bodyAsText ? {} : JSON.parse(rsp.bodyAsText);
	this.variables = JSON.parse(rsp.bodyAsText ?? "{}") as Record<string, string>;

[Unified Recorder] Support "variables" in recordings with proxy-tool #18585

[Unified Recorder] Support "variables" in recordings with proxy-tool #18585

Conversation

HarshaNalluru commented Nov 9, 2021 • edited Loading

Issue #18580

What?

Internals(How does it work?):

seankane-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

sadasant left a comment

Choose a reason for hiding this comment

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

HarshaNalluru commented Nov 11, 2021

HarshaNalluru commented Nov 9, 2021 •

edited

Loading