[Key Vault] Fixing min-max tests by moving test utilities to test/public by sadasant · Pull Request #19959 · Azure/azure-sdk-for-js

sadasant · 2022-01-21T00:16:58Z

This PR focuses on moving the test utilities on the Key Vault packages from test/utils to test/public/utils, with the caveat that any references to non-public API would need to either be copied to test/public/ somewhere, or moved to test/internal.

Fixes #14955

KarishmaGhiya

sorry i didn't notice the ci failing. had approved since the min-max tests pass

sadasant · 2022-01-22T01:10:13Z

/azp run js - keyvault-certificates - tests

azure-pipelines · 2022-01-22T01:10:25Z

Azure Pipelines successfully started running 1 pipeline(s).

maorleger

Thanks so much for taking this effort! I left a few comments but I have some open-ended questions / concerns if you don't mind

Requiring utils to be in public folder feels implicit and brittle since nothing lets me know if I don't follow this convention. I can definitely imagine a situation where I break this in the future. Are there any options to encode this into a linter or CI validation or something? @KarishmaGhiya
It sounds like the outcome of not doing this is that test coverage gets missed in min/max testing is that right? If so, let's run a live test against this PR for each of the KV libraries and make sure that our min/max builds run significantly more tests now than in the latest main scheduled builds - if we didn't improve maybe we don't need to bother?

maorleger · 2022-01-26T17:01:21Z

sdk/keyvault/keyvault-certificates/test/public/utils/common.ts

+/**
+ * The latest supported KeyVault service API version
+ */
+export const LATEST_API_VERSION = "7.3-preview";


Can we infer this from https://github.com/Azure/azure-sdk-for-js/pull/19959/files#diff-8182aee8eae78191aaa9301924729aafe2786e1f58fd5bd5bc31af8c28405d23R43 instead of redefining it? I just worry about having yet another place to change you know?

maorleger · 2022-01-26T17:02:58Z

sdk/keyvault/keyvault-keys/test/internal/utils/internalTestClient.ts

+import { BeginRestoreKeyBackupOptions } from "../../public/utils/lro/restore/operation";
+import TestClient from "../../public/utils/testClient";
+
+export default class InternalTestClient extends TestClient {


Where is this used? If only like a one or two places can we just remove this class and inline this logic in the test or make a helper function in the test file?

Ok I like the idea of inlining this!

We talked over teams. Here’s what I’ll do:

This class is intended to help us do some common functionality on many tests. This specific extension is only used in one test, so we can grab the internals of this extra function and move it to the test directly, then remove this class.

I’ll do that!

sdk/keyvault/keyvault-keys/test/public/utils/common.ts

maorleger · 2022-01-26T17:06:22Z

sdk/keyvault/keyvault-keys/test/public/utils/testAuthentication.ts

+export async function authenticate(
+  that: Context,
+  version: string,
+  testClientMaker?: (client: KeyClient) => TestClientInterface


This feels a little odd to me - what is this used for? Sorry, I forgot 😄

it’s to allow for the replacement of the TestClient with the InternalTestClient. The TestClient at the moment has some private references.

maorleger · 2022-01-26T17:09:40Z

sdk/keyvault/keyvault-keys/test/public/utils/testClient.ts

+  purgeKey: (keyName: string) => Promise<void>;
+  flushKey: (keyName: string) => Promise<void>;
+}
+export default class TestClient implements TestClientInterface {


I have a proposal! instead of formatName let's just use recorder.getUniqueName. And then let's just get rid of calls to flushKey and purgeKey - I've been doing that to make our tests faster since the KV gets created and deleted anyway. Then we won't need the testClient at all. What do you think?

I love that idea! Thank you 🙏 I’ll take it

Wait, the main point of this test is to restore a key backup, so the unique name won’t help 🤔 I do the purge to then recover the backup. Let’s sync over teams!

maorleger · 2022-01-26T17:10:20Z

sdk/keyvault/keyvault-secrets/test/public/utils/common.ts

+/**
+ * The latest supported KeyVault service API version
+ */
+export const LATEST_API_VERSION = "7.3-preview";


We can infer this from https://github.com/Azure/azure-sdk-for-js/pull/19959/files#diff-c3564ec34ef1be7b6ff757d6352c4dc035b76e9fea338b9dad4298414d2a8b03L27 as well - sorry for repeating myself I just wanted to make sure it doesn't get missed

sadasant · 2022-01-26T18:46:19Z

@maorleger the min-max tests at the moment only includes the public tests it can run (meaning, that don’t have any internal reference). This PR does increase the number of min-max tests that the Key Vault projects run.

The min-max test tooling is rather simple, which should help with maintenance, but I agree that it would be interesting to do a re-engeneering of it, to behave more like our samples tooling.

…me file

check-enforcer · 2022-01-26T23:45:30Z

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

maorleger

🚀 thank you for taking this on!

maorleger · 2022-01-27T23:57:51Z

sdk/keyvault/keyvault-keys/test/public/utils/testClient.ts

+import { testPollerProperties } from "./recorderUtils";
+import { KeyClient } from "../../../src";
+
+export interface TestClientInterface {


Nit: I think this interface is no longer necessary

maorleger · 2022-01-27T23:58:23Z

/azp run js - keyvault-keys - tests

maorleger · 2022-01-27T23:58:33Z

/azp run js - keyvault-certificates - tests

sadasant · 2022-01-28T22:15:48Z

/azp run js - keyvault-keys - tests

azure-pipelines · 2022-01-28T22:16:00Z

Azure Pipelines successfully started running 1 pipeline(s).

sadasant · 2022-01-29T02:06:51Z

CI Passed! I’ll merge this.

add x-ms-identifiers for healthbot (Azure#19959)

sadasant added 4 commits January 21, 2022 00:07

changes on kv-secret

05dbcae

kv-keys

95fdb72

kv-certificates

7532b47

certificates and admin

d918606

sadasant requested a review from KarishmaGhiya January 21, 2022 00:16

sadasant self-assigned this Jan 21, 2022

ghost added the KeyVault label Jan 21, 2022

fixing paths on public tests to not reference anything that isnt public

ec27c17

KarishmaGhiya approved these changes Jan 21, 2022

View reviewed changes

KarishmaGhiya requested changes Jan 21, 2022

View reviewed changes

additional fix necessary in KeyVault Keys

6509b15

sadasant requested a review from KarishmaGhiya January 21, 2022 22:21

sadasant marked this pull request as ready for review January 21, 2022 22:21

sadasant requested a review from maorleger as a code owner January 21, 2022 22:21

sadasant changed the title ~~[Key Vault] Fixing 14955~~ [Key Vault] Fixing min-max tests by moving test utilities to test/public Jan 21, 2022

KarishmaGhiya approved these changes Jan 24, 2022

View reviewed changes

maorleger reviewed Jan 26, 2022

View reviewed changes

sadasant added 2 commits January 26, 2022 23:30

deriving the SERVICE_VERSION from the serviceVersions array in the sa…

eb1c60c

…me file

inlining the InternalTestClient

686afcf

sadasant added 2 commits January 26, 2022 23:53

Merge remote-tracking branch 'Azure/main' into fix14955

41c0483

lint fix

d833da3

sadasant requested a review from maorleger January 27, 2022 22:09

inlining the restore operation

7d7f743

maorleger approved these changes Jan 27, 2022

View reviewed changes

sadasant requested review from EmmaZhu, HarshaNalluru, MaryGao, XiaoningLiu, bterlson, deyaaeldeen, dw511214992, jeremymeng, jiacfan, joheredi, ljian3377, mikeharder, nguerrera, praveenkuttappan, qiaozha, ramya-rao-a, timovv, vinjiang, weshaggard and xirzec as code owners January 28, 2022 21:18

using error.name instead of instanceof

8d063af

sadasant force-pushed the fix14955 branch from c8890db to 8d063af Compare January 28, 2022 21:33

sadasant added 2 commits January 28, 2022 21:40

Merge remote-tracking branch 'Azure/main' into fix14955

5f004f3

formatting in schema registry

f92122e

sadasant merged commit cba4a9b into Azure:main Jan 29, 2022

sadasant deleted the fix14955 branch January 29, 2022 02:07

azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Aug 10, 2022

CodeGen from PR 19959 in Azure/azure-rest-api-specs

e4f30d3

add x-ms-identifiers for healthbot (Azure#19959)

Conversation

sadasant commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KarishmaGhiya left a comment

Choose a reason for hiding this comment

Uh oh!

sadasant commented Jan 22, 2022

Uh oh!

azure-pipelines bot commented Jan 22, 2022

Uh oh!

maorleger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasant commented Jan 26, 2022

Uh oh!

check-enforcer bot commented Jan 26, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

maorleger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maorleger commented Jan 27, 2022

Uh oh!

maorleger commented Jan 27, 2022

Uh oh!

sadasant commented Jan 28, 2022

Uh oh!

azure-pipelines bot commented Jan 28, 2022

Uh oh!

sadasant commented Jan 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

sadasant commented Jan 21, 2022 •

edited

Loading