[App Config] Migrate the tests to use the new unified recorder #18896

HarshaNalluru · 2021-12-01T02:34:55Z

Follows #18817 and #19210 and migrates AppConfig tests to use the new unified recorder.

… harshan/issue/17042

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

… harshan/issue/17042

… harshan/unified-recorder/migrate-app-config-way-2

HarshaNalluru · 2021-12-29T23:00:44Z

sdk/appconfiguration/app-configuration/test/public/utils/testHelpers.ts

+// allow loading from a .env file as an alternative to defining the variable
+// in the environment


remove

Suggested change

// allow loading from a .env file as an alternative to defining the variable

// in the environment

HarshaNalluru · 2021-12-29T23:04:05Z

sdk/appconfiguration/app-configuration/test/internal/http.spec.ts

@@ -138,160 +139,162 @@ describe("http request related tests", function () {
    });
  });

+  // Commenting out the tests with nock.Scope


handle commented tests in this or a followup PR

HarshaNalluru · 2021-12-29T23:04:18Z

sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts

+  /**
+   * The HttpClient that will be used to send HTTP requests.
+   */
+  httpClient?: HttpClient;


API addition

witemple-msft

Couple of thoughts/questions.

Does recorder.configureClient only work for the corev2 scenario?

witemple-msft · 2022-01-03T18:18:59Z

sdk/appconfiguration/app-configuration/review/app-configuration.api.md

@@ -39,6 +40,7 @@ export class AppConfigurationClient {

 // @public
 export interface AppConfigurationClientOptions {
+    httpClient?: HttpClient;


Might be a blessing in disguise that we haven't added this to app config yet. It might be possible to migrate to corev2 without any breaking changes, not even little ones.

Hmmm, with that logic we'll only migrate core-v2 packages to use the new recorder. (Since, almost none of our core-v1 packages expose httpClient if I'm not wrong, with a few exceptions)
And wait for the core-v1 package to be migrated to core-v2 before migrating to the new recorder.

?

witemple-msft · 2022-01-03T18:21:13Z

sdk/appconfiguration/app-configuration/test/internal/http.spec.ts

+        createAppConfigurationClientForTests(recorder.configureClientOptionsCoreV1({})) ||
+        this.skip();


I'm a little confused by this || this.skip() here. What is this for?

This was retained from what was there before.
I can update it appropriately.

witemple-msft · 2022-01-03T18:22:36Z

sdk/appconfiguration/app-configuration/test/internal/http.spec.ts

+  //         syncTokens: syncTokens
+  //       }) || this.skip();
+
+  //     nock.recorder.clear();


Could you elaborate on why these tests interface with nock directly and whether/what the alternative is for this use case with the new recorder?

Richard added those tests, I need to review them again to see if I should keep it or remove/update.

witemple-msft · 2022-01-03T18:23:58Z

sdk/appconfiguration/app-configuration/test/internal/node/throttlingRetryPolicy.spec.ts

+      if (!(error instanceof Error)) {
+        throw new Error("Error of unexpected kind is thrown");
+      }


In situations like this where you catch something you can't handle, IMO it's best to just re-throw it.

Suggested change

if (!(error instanceof Error)) {

throw new Error("Error of unexpected kind is thrown");

}

if (!(error instanceof Error)) {

throw error;

}

witemple-msft · 2022-01-03T18:26:58Z

sdk/appconfiguration/app-configuration/test/public/featureFlag.spec.ts

+      client =
+        createAppConfigurationClientForTests(recorder.configureClientOptionsCoreV1({})) ||
+        this.skip();
+      recorder.variable("name-1", `${getRandomNumber()}`);


Not sure if the recorder is wired up for this, but something like const name = recorder.variable("name-1", getRandomNumber().toString()); seems a little more natural to me. Does recorder.variable return the stored value if an initializer is passed?

It does return

witemple-msft · 2022-01-03T18:28:04Z

sdk/appconfiguration/app-configuration/test/public/index.readonlytests.spec.ts

-    client = createAppConfigurationClientForTests() || this.skip();
+    recorder = new Recorder(this.currentTest);
+    await recorder.start(recorderStartOptions);
+    recorder.variable("readOnlyTests", `readOnlyTests-${Math.ceil(Math.random() * 1000 + 1000)}`);


Didn't I see a getRandomNumber function somewhere around?

witemple-msft · 2022-01-03T18:30:30Z

sdk/appconfiguration/app-configuration/test/public/index.spec.ts

-      const key = recorder.getUniqueName("noLabelTests");
-
+      recorder.variable("noLabelTests", `noLabelTests-${getRandomNumber()}`);
+      key = recorder.variable("noLabelTests");


This binding of key is a little weird to me. The name is bound in the outer describe scope, assigned in the beforeEach, and then reassigned here?

If this one test is unique, I would avoid re-using the name from the outer scope and just create a local constant. I'm not sure I follow what this test is doing with key, though.

witemple-msft · 2022-01-03T18:31:49Z

sdk/appconfiguration/app-configuration/test/public/secretReference.spec.ts

+    recorder.variable("key-1", `key-1-${getRandomNumber()}`);
+    recorder.variable(
+      "random-string-1",
+      `random-string-1-${Math.ceil(Math.random() * 1000 + 1000)}`


Is there something semantically different about this way of constructing a random and the getRandomNumber function?

witemple-msft · 2022-01-03T18:33:34Z

sdk/appconfiguration/app-configuration/test/public/utils/testHelpers.ts

    }
  }

  return {
-    credential: new DefaultAzureCredential(),
+    credential: createTestCredential(options),


How were we able to use DAC before, but were unable to use it in the implementation of createTestCredential?

I believe the test was skipped(in the browser).

Richard added that long ago probably with the hope to update to something more useful, but never got to it maybe.

ghost · 2022-03-11T10:02:52Z

Hi @HarshaNalluru. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

ghost · 2022-03-18T11:01:57Z

Hi @HarshaNalluru. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass.

[Hub Generated] Review request for Microsoft.VirtualMachineImages to add version stable/2022-02-14 (Azure#18896) * Adds base for updating Microsoft.VirtualMachineImages from version stable/2021-10-01 to version 2022-02-14 * Updates readme * Updates API version in new specs and examples * adding 2022 changes * fixed proxy resource path * added object to ImageTemplate* * added object to valide and prettier to examples * added identifier * removed format integer 32 for breaking change * added back format integer 64, to be in compliance for lintdiff and breaking change * adding back int32 after getting jeffrey richters approval

HarshaNalluru added 30 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

clean up

4bbc37e

gets RootLocation

fc25043

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

4f0f2e6

cleanup

09a20ea

testProxyUtils.ts

e69fa22

checkpoint

be70341

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

"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

default options

4469c36

--single-run

d204966

remove dev-tool shortcut

b986e5f

HarshaNalluru added 10 commits December 27, 2021 20:52

build failures

5c68b9b

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

5cad2f8

… harshan/unified-recorder/migrate-app-config-way-2

lock file + package.json update

e096d23

revert dev-tool changes

a3a108e

minor fixes

969ab29

recordings

4e47758

fix recording

aeb9510

eslint fixes

e57b3c2

formatting

d126413

more formatting

c9c1b01

HarshaNalluru marked this pull request as ready for review December 29, 2021 23:00

HarshaNalluru requested review from bterlson, deyaaeldeen, jeremymeng, mikeharder, praveenkuttappan, ramya-rao-a, sadasant, timovv, witemple-msft and xirzec as code owners December 29, 2021 23:00

HarshaNalluru commented Dec 29, 2021

View reviewed changes

witemple-msft reviewed Jan 3, 2022

View reviewed changes

ghost added the no-recent-activity There has been no recent activity on this issue. label Mar 11, 2022

ghost closed this Mar 18, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Config] Migrate the tests to use the new unified recorder #18896

[App Config] Migrate the tests to use the new unified recorder #18896

HarshaNalluru commented Dec 1, 2021 •

edited

Loading

HarshaNalluru Dec 29, 2021

HarshaNalluru Dec 29, 2021

HarshaNalluru Dec 29, 2021

witemple-msft left a comment

witemple-msft Jan 3, 2022

harsha-nalluru Jan 3, 2022 •

edited

Loading

witemple-msft Jan 3, 2022

HarshaNalluru Jan 3, 2022

witemple-msft Jan 3, 2022

harsha-nalluru Jan 3, 2022 •

edited

Loading

witemple-msft Jan 3, 2022

witemple-msft Jan 3, 2022

HarshaNalluru Jan 3, 2022

witemple-msft Jan 3, 2022

harsha-nalluru Jan 3, 2022

witemple-msft Jan 3, 2022

witemple-msft Jan 3, 2022

witemple-msft Jan 3, 2022

harsha-nalluru Jan 3, 2022 •

edited

Loading

ghost commented Mar 11, 2022

ghost commented Mar 18, 2022

		// allow loading from a .env file as an alternative to defining the variable
		// in the environment

		createAppConfigurationClientForTests(recorder.configureClientOptionsCoreV1({})) \|\|
		this.skip();

[App Config] Migrate the tests to use the new unified recorder #18896

[App Config] Migrate the tests to use the new unified recorder #18896

Conversation

HarshaNalluru commented Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this 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

harsha-nalluru Jan 3, 2022 • edited Loading

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

harsha-nalluru Jan 3, 2022 • edited Loading

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

harsha-nalluru Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

ghost commented Mar 11, 2022

ghost commented Mar 18, 2022

HarshaNalluru commented Dec 1, 2021 •

edited

Loading

harsha-nalluru Jan 3, 2022 •

edited

Loading

harsha-nalluru Jan 3, 2022 •

edited

Loading

harsha-nalluru Jan 3, 2022 •

edited

Loading