Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Recorder] Schema registry - migrate tests to the new recorder #19628

Merged
merged 30 commits into from
Jan 24, 2022
Merged

[Recorder] Schema registry - migrate tests to the new recorder #19628

merged 30 commits into from
Jan 24, 2022

Conversation

harsha-nalluru
Copy link
Contributor

@harsha-nalluru harsha-nalluru commented Jan 5, 2022 •
edited by HarshaNalluru
Loading

Based on the migration guide at #19210

Fixes #17999

@ghost ghost added Schema Registry customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Jan 5, 2022
@ghost
Copy link

ghost commented Jan 5, 2022

Thank you for your contribution harsha-nalluru! We will review the pull request and get back to you soon.

@ghost
Copy link

ghost commented Jan 5, 2022 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@harsha-nalluru harsha-nalluru changed the title [Recorder] Schema registry migrate tests to the new recorder [Recorder] Schema registry - migrate tests to the new recorder Jan 5, 2022
deyaaeldeen
deyaaeldeen reviewed Jan 5, 2022
View reviewed changes
Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the new setup! I am impressed with how clean the new recordings are! I left a few comments.

Also, I am seeing this error in CI:

 1) SchemaRegistryClient
       "before each" hook for "sets fully qualified name space in constructor":
     Error: SCHEMA_REGISTRY_ENDPOINT is not defined
      at createRecordedClient (test\public\utils\recordedClient.ts:26:44)
      at Context.<anonymous> (test\public\schemaRegistry.spec.ts:68:29)
      at processImmediate (internal/timers.js:461:21)

Returned error code: 2
  2) SchemaRegistryClient
"@azure/schema-registry" failed to build.
       "after each" hook for "sets fully qualified name space in constructor":
     TypeError: Cannot read property 'stop' of undefined
      at Context.<anonymous> (test\public\schemaRegistry.spec.ts:94:20)
      at processImmediate (internal/timers.js:461:21)

@timovv timovv self-requested a review January 5, 2022 21:56
@harsha-nalluru harsha-nalluru mentioned this pull request Jan 6, 2022
Closed
19 tasks
deyaaeldeen
deyaaeldeen approved these changes Jan 6, 2022
View reviewed changes
Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very excited to try this, unlike with the old recorder, I am hopeful the recording experience will be smooth sailing from here.

witemple-msft
witemple-msft reviewed Jan 6, 2022
View reviewed changes
Copy link
Member

@witemple-msft witemple-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts. It looks good but I'm concerned about the fact that we don't expose pipeline here but have seemingly assumed in the recorder that this will always be the case.

sdk/schemaregistry/schema-registry/test/public/schemaRegistry.spec.ts Outdated
schema = {
name: "azsdk_js_test",
groupName: env.SCHEMA_REGISTRY_GROUP,
groupName: getEnvironmentVariable("SCHEMA_REGISTRY_GROUP"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a regression to have to call a function to get an environment variable instead of being able to access it as a property of an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I make env to be Record<string, string> then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@witemple-msft you do not have to call the function, you just need to check whether the field you're accessing exists.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I make env to be Record<string, string> then?

Okay I see now that the reason the function exists is because it asserts that an environment variable is defined, but that you can still get the environment variable from env, it just might be undefined. I would not change it to Record<string, string>.

It'd be helpful if the function name somehow conveys that it is not just getting the value of an environment variable, but will throw if it is not defined i.e. assertEnvironmentVariable, or getEnvStrict, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Calling it assertEnvironmentVariable now.

Any objections? @deyaaeldeen @jeremymeng

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harsha-nalluru Sounds good to me, thanks for iterating over this!

@harsha-nalluru harsha-nalluru mentioned this pull request Jan 7, 2022
Closed
deyaaeldeen
deyaaeldeen approved these changes Jan 7, 2022
View reviewed changes
Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too excited so I am approving for the second time 😅

jeremymeng
jeremymeng reviewed Jan 7, 2022
View reviewed changes
sdk/schemaregistry/schema-registry/package.json
"integration-test:browser": "karma start --single-run",
"integration-test:node": "nyc mocha -r esm --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 5000000 --full-trace \"dist-esm/test/{,!(browser)/**/}*.spec.js\"",
"integration-test:browser": "dev-tool run test:browser",
"integration-test:node": "dev-tool run test:node-js-input -- --timeout 5000000 'dist-esm/test/**/*.spec.js'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The {,!(browser)/**/} part is gone?

Copy link
Contributor Author

@harsha-nalluru harsha-nalluru Jan 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were no browser specific tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I had some issue with } in the command and I did not care to investigate further. 😋

sadasant
sadasant approved these changes Jan 10, 2022
View reviewed changes
Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good!

@HarshaNalluru HarshaNalluru enabled auto-merge (squash) January 24, 2022 21:08
@HarshaNalluru HarshaNalluru merged commit 8018321 into Azure:main Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sadasant sadasant sadasant approved these changes

@deyaaeldeen deyaaeldeen deyaaeldeen approved these changes

@bterlson bterlson Awaiting requested review from bterlson

@nguerrera nguerrera Awaiting requested review from nguerrera

@ramya-rao-a ramya-rao-a Awaiting requested review from ramya-rao-a

@xirzec xirzec Awaiting requested review from xirzec

@benbp benbp Awaiting requested review from benbp

@praveenkuttappan praveenkuttappan Awaiting requested review from praveenkuttappan

@timovv timovv Awaiting requested review from timovv

@HarshaNalluru HarshaNalluru Awaiting requested review from HarshaNalluru

@witemple-msft witemple-msft Awaiting requested review from witemple-msft

@jeremymeng jeremymeng Awaiting requested review from jeremymeng

Assignees
No one assigned
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. Schema Registry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Schema Registry - Move to the new proxy server based recorder
7 participants
@harsha-nalluru @sadasant @xirzec @deyaaeldeen @jeremymeng @witemple-msft @HarshaNalluru