Record and Playback - refactoring by HarshaNalluru · Pull Request #4281 · Azure/azure-sdk-for-js

HarshaNalluru · 2019-07-11T23:24:00Z

Currently, the base recorder which allows recording the tests and playing them back is being duplicated across packages(for both node and browser that I and Caio have worked on).

Example - recorder.ts for storage-queue package

In an effort to avoid the duplication of the recorder, this PR attempts to unify the recorder modules and provide a common utils library in the repo.

So, this new package "@azure/test-utils-recorder" can be added as a devDependency in an sdk (for example, storage-queue) to leverage the functionalities provided by the "recorder" to record the tests and generate recordings and to playback those recordings.

HarshaNalluru · 2019-07-11T23:59:47Z

sdk/test-utils/recorder/src/baseRecorder.ts

+const skip = [
+  // Abort
+  "browsers/aborter/recording_should_abort_after_aborter_timeout.json"
+];


The eventual plan is to remove the skip list - #4084 (comment)
Will work out in a future PR.

HarshaNalluru · 2019-07-12T00:46:23Z

sdk/test-utils/recorder/src/baseRecorder.ts

+if (isPlayingBack) {
+  // Providing dummy values to avoid the error
+  env.ACCOUNT_NAME = "fakestorageaccount";
+  env.ACCOUNT_KEY = "aaaaa";


More work on ENV variables
#4284

HarshaNalluru · 2019-07-12T00:46:32Z

sdk/test-utils/recorder/src/baseRecorder.ts

+      new RegExp(env.ACCOUNT_NAME, "g"),
+      "fakestorageaccount"
+    );
+    if (env.ACCOUNT_KEY) {


More work on ENV variables
#4284

…idelines

HarshaNalluru · 2019-08-14T20:27:11Z

sdk/test-utils/recorder/src/baseRecorder.ts

+// Nise module does not have a native implementation of record/playback like Nock does
+// This class overrides requests' 'open', 'send' and 'onreadystatechange' functions, adding our own code to them to deal with requests
+export class NiseRecorder extends BaseRecorder {
+  private readonly sasQueryParameters = ["se", "sig", "sp", "spr", "srt", "ss", "st", "sv"];


This is to filter sasQueryParameters in the browser tests for storage sdks.
Code related to this doesn't affect if such query params are not present.
(Meaning.. for example, this doesn't affect keyvault sdks - #4749 (comment))

Will work in a future PR to update this make the recorder more generic.

/cc - @sadasant

These hardcoded parameters should be passed into the library rather than part of the library itself. Different services may have their own query parameters, potentially.

Addressed below - #4281 (comment)

sophiajt · 2019-08-21T22:27:49Z

sdk/test-utils/recorder/GUIDELINES.md

+  For example
+
+  ```typescript
+  describe("Aborter", () => {


I thought we weren't using this name

Hmm. We haven't updated the title in storage test-suites.

Updated this to "Some Random Test Suite" here.

sophiajt · 2019-08-21T22:29:39Z

sdk/test-utils/recorder/GUIDELINES.md

+
+- The above methods can be called from the `before` section so that the tests can leverage the environment variables.
+
+- Currently, the environment variables for storage-packages are managed by the recorder. `ACCOUNT_NAME` is replaced with `"fakestorageaccount"`, secret part of `ACCOUNT_SAS` and `ACCOUNT_KEY` are replaced with `"aaaaa"`.


Storage-specific stuff should live outside of the recorder package.

sophiajt · 2019-08-21T22:32:58Z

sdk/test-utils/recorder/src/baseRecorder.ts

+    env.ACCOUNT_KEY = "aaaaa";
+    env.ACCOUNT_SAS = "aaaaa";
+    env.STORAGE_CONNECTION_STRING = `DefaultEndpointsProtocol=https;AccountName=${
+      env.ACCOUNT_NAME


Storage-specific functionality should be outside of the recorder. I see the TODO, but it'd be good to make sure we're landing something general first, and then we can land PRs for storage and kv to use it.

sophiajt · 2019-08-21T22:34:08Z

sdk/test-utils/recorder/src/baseRecorder.ts

+      "/recording_" +
+      this.formatPath(testTitle) +
+      "." +
+      ext;


Can we test for node?

sophiajt · 2019-08-21T22:36:12Z

sdk/test-utils/recorder/src/baseRecorder.ts

+// Nise module does not have a native implementation of record/playback like Nock does
+// This class overrides requests' 'open', 'send' and 'onreadystatechange' functions, adding our own code to them to deal with requests
+export class NiseRecorder extends BaseRecorder {
+  private readonly sasQueryParameters = ["se", "sig", "sp", "spr", "srt", "ss", "st", "sv"];


These hardcoded parameters should be passed into the library rather than part of the library itself. Different services may have their own query parameters, potentially.

sophiajt · 2019-08-21T22:37:03Z

sdk/test-utils/recorder/src/baseRecorder.ts

+      responseHeaders[key] = value;
+    }
+
+    // We're not storing SAS Query Parameters because they may contain sensitive information


It seems like the service should send you the additional set of parameters that may contain sensitive information

Updated by exporting a new method called skipQueryParams, where the params can be provided by the sdk itself, for example, SAS query params can be provided by storage-queue package.

sophiajt · 2019-08-21T22:40:12Z

sdk/test-utils/recorder/src/baseRecorder.ts

+          const reqStateChange = req.onreadystatechange;
+          req.onreadystatechange = function() {
+            // Record the request once the response is obtained
+            if (req.readyState === 4) {


Can you say more about what readyState is, and what in particular 4 is? Could this be an enum instead?

Added some comments stating what readyState refers to.

I mean more the hard-coded 4. Why is it 4 and not 3 or 5?

More info - https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/readyState

89a9188 - Added the following comment
readyState = 4 refers to the completion of the operation.

Should I talk more about it?

Ah okay, so there isn't an official enumeration, just the number. It's fine to leave it as is, but you may want to link to the documentation for people who try to read the code later

sophiajt · 2019-08-21T22:42:25Z

sdk/test-utils/recorder/src/recorder.ts

+ * @export
+ * An interface that allows recording and playback capabilities for the tests in Azure TS SDKs.
+ */
+export interface Recorder {


If only one Recorder type is exposed, I think it's fine to keep the name.

…er minor comments

HarshaNalluru · 2019-08-22T22:28:57Z

@jonathandturner and @sadasant

Tested the new @azure/test-utils-recorder with keyvault-secrets package

Imported the new recorder into a branch and updated keyvault-secrets package to leverage the @azure/test-utils-recorder
Branch (as a PR to view the changes easily) - @azure/test-utils-recorder in keyvault-secrets package HarshaNalluru/azure-sdk-for-js#15

Findings/comments/questions/suggestions

unit-test:browser is "echo skipped" - Is there a reason behind skipping the unit tests for the browser ?
Deleted the recordings
Generated the recordings for node tests - Successful
Playback is also successful for node tests - Successful
Some of the tests have same title and are in the same test-suite - this needs to be updated
I've observed that we need to set env variables in the .env file and using set var=value
We can update the code to import env variables from .env file using dotenv all the time and reduce the complexity of doing set var=value each time we run the tests.
One suggestion to update the test:node command to
"test:node": "npm run clean && npm run build:test && npm run integration-test:node", and use it instead of having to run both build:test and integration:test
[ I kept forgetting that I need to run build:test before running integration-test:node"]
Browser tests
- With @azure/test-utils-recorder, tests executed successfully in "record" mode - generated recordings as well.
- In the playback mode, two tests succeeded and then exited. Reason being the usage of abort in the test. It is a known issue and we skip the test in browsers during playback mode.
- Conclusion - Record and playback are successful for the browser tests

/cc - @ramya-rao-a

KarishmaGhiya · 2019-08-23T21:24:32Z

sdk/test-utils/recorder/package.json

+    "integration-test:node": "echo skipped",
+    "integration-test": "npm run integration-test:node && npm run integration-test:browser",
+    "lint:fix": "eslint \"src/**/*.ts\" \"test/**/*.ts\" -c ../../.eslintrc.json --fix --fix-type [problem,suggestion]",
+    "lint": "eslint -c ../../.eslintrc.json src test --ext .ts -f node_modules/eslint-detailed-reporter/lib/detailed.js -o template-lintReport.html || exit 0",


Can you submit another PR and change the lint step to :
"lint": "eslint -c ../../.eslintrc.json src test --ext .ts -f html -o recorder-lintReport.html || exit 0",

HarshaNalluru added 19 commits July 10, 2019 17:52

update rush.json with the new package

b5c0f39

porting the latest Guidelines from feature/storage

a29370d

porting the latest guidelines from feature/storage branch

e63ec63

add readme - test utils - Record and Playback

253679c

Add disclaimer - mocha and karma

84a1573

removed the dependency of core-http

5396927

remove storage utils dependency

4a9269b

add package.json

404e8f2

cherry picked commit from feature/storage branch

979c144

Merge branch 'QureyStringPackage' into CommonRecorder

1bc7c35

pnpm-lock updated

b727686

simplify recorder.ts with utils.ts

e4ffd2e

removed dotenv

b8958f3

Improved delay module

e6b3f10

remove delay from utils.ts file

0639d01

keep only the final recorder module in recorder.ts

387cfd8

separate customConsoleLog.ts

2c74e8a

decouple base recorder from recorder

662c70e

add some return types

8836ea6

HarshaNalluru added the Client This issue points to a problem in the data-plane of the library. label Jul 11, 2019

HarshaNalluru requested a review from kinelski July 11, 2019 23:24

HarshaNalluru self-assigned this Jul 11, 2019

resolve merge conflicts with master

6c92d5d

HarshaNalluru requested a review from sadasant July 11, 2019 23:40

HarshaNalluru added 2 commits July 11, 2019 16:56

update skip list

ec1c239

ignore readme checks

577c6b0

HarshaNalluru commented Jul 11, 2019

View reviewed changes

HarshaNalluru requested a review from ramya-rao-a July 12, 2019 00:38

HarshaNalluru commented Jul 12, 2019

View reviewed changes

isRecording -> isRecordMode, isPlayingBack -> isPlaybackMode

725baa0

HarshaNalluru requested review from KarishmaGhiya and mikeharder as code owners August 6, 2019 22:24

HarshaNalluru added 6 commits August 12, 2019 21:26

runtime: "node" | "browsers" - union type

f121be7

testHierarchy -> testSuiteTitle

bd97933

runtime -> platform

5c789ae

importNock constant to importNockStatement

12f0152

update the escapeRegExp function

809acc7

Elaborating beforeeach, aftereach details and minor improvements - Gu…

55c2c53

…idelines

HarshaNalluru commented Aug 14, 2019

View reviewed changes

HarshaNalluru mentioned this pull request Aug 14, 2019

Record and Playback - Refactoring and Improvements #4229

Closed

62 tasks

HarshaNalluru changed the title ~~[WIP] Record and Playback - refactoring~~ Record and Playback - refactoring Aug 14, 2019

HarshaNalluru added 3 commits August 21, 2019 14:15

merge upstream/master - resolve conflicts

46e36e7

Merge remote-tracking branch 'upstream/master' into CommonRecorder

2fd17df

regenerate pnpm-lock file

1974538

sophiajt reviewed Aug 21, 2019

View reviewed changes

HarshaNalluru added 4 commits August 22, 2019 11:02

Aborter -> Some Random Test Suite

82e17a0

Merge remote-tracking branch 'upstream/master' into CommonRecorder

eacc8c5

Addressed Jonathan's comments - removed storage specific code and oth…

b2073c9

…er minor comments

More info on readyState

89a9188

HarshaNalluru requested a review from sophiajt August 22, 2019 19:30

merge upstream/master - resolve merge conflicts

9c74e25

HarshaNalluru mentioned this pull request Aug 22, 2019

Record And Playback - Breaking changes in mocha@6 #4850

Closed

Copying Mike's fix from Azure#4859 for mocha@6

604b420

sophiajt approved these changes Aug 22, 2019

View reviewed changes

HarshaNalluru merged commit 81fdd91 into Azure:master Aug 23, 2019

This was referenced Aug 23, 2019

Refactor test/utils/recorder.ts for RecordAndPlayback #3939

Closed

Record and Playback - Add proper docs for console.logs karma config #4276

Closed

KarishmaGhiya reviewed Aug 23, 2019

View reviewed changes


		- The above methods can be called from the `before` section so that the tests can leverage the environment variables.

		- Currently, the environment variables for storage-packages are managed by the recorder. `ACCOUNT_NAME` is replaced with `"fakestorageaccount"`, secret part of `ACCOUNT_SAS` and `ACCOUNT_KEY` are replaced with `"aaaaa"`.

Conversation

HarshaNalluru commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HarshaNalluru Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

HarshaNalluru Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

HarshaNalluru Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

HarshaNalluru Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

HarshaNalluru Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

HarshaNalluru Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

HarshaNalluru commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

HarshaNalluru commented Jul 11, 2019 •

edited

Loading

HarshaNalluru Jul 11, 2019 •

edited

Loading

HarshaNalluru Aug 14, 2019 •

edited

Loading

HarshaNalluru Aug 21, 2019 •

edited

Loading

HarshaNalluru Aug 22, 2019 •

edited

Loading

HarshaNalluru Aug 22, 2019 •

edited

Loading

HarshaNalluru Aug 22, 2019 •

edited

Loading

HarshaNalluru commented Aug 22, 2019 •

edited

Loading