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.

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

[Perf Framework] Restructuring the perf test folders - Proposal 2 #13770

Merged
48 commits merged into from
Feb 23, 2021

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented Feb 12, 2021

Description

To allow testing the older versions of track-2 SDKs and for smoother automation with the pipelines, we are moving away from the in-place perf tests to independent projects under the service folders. This change doesn't affect how the perf tests are written.

The main change here is that the track 2 perf tests are made into a project maintained by "rush" as opposed to in-place tests.
Track 1 tests are not managed by "rush", it is an npm project.

Old

  • sdk/storage/storage-blob/test/perfstress/track-1/ - npm project (but imports the perf framework)
  • sdk/storage/storage-blob/test/perfstress/track-2/ - depends on src code like the regular tests

New

  • sdk/storage/perf-tests/storage-blob-track-1/ - Independent npm project (but imports the perf framework)
  • sdk/storage/perf-tests/storage-blob/              - New project maintained through "rush" (depends on storage-blob on master)

One key change is that the rush property - "projectFolderMaxDepth" is being changed from 3 to 4 to allow creating new projects at 4 levels down from the root.

The following walk-through for executing perf tests uses storage-blob as an example. (Check #13740 for proposal 1 - old.)

Testing Track 1

  • rush update
  • Navigate to sdk\storage\perf-tests\storage-blob-track-1
  • Run npm run setup (Builds the perf package and installs it for track-1 perf tests)
  • Add .env file as suggested in the readme
  • Run the tests as suggested by readme, example npm run perf-test:node -- StorageBlobDownloadTest --warmup 2 --duration 7 --iterations 2 --parallel 2

Testing Track 2 - unpublished(current master)

  • rush update
  • Navigate to sdk\storage\perf-tests\storage-blob
  • rush build -t perf-test-storage-blob
  • Add .env file as suggested in the readme
  • Run the tests as suggested by readme, example npm run perf-test:node -- StorageBlobDownloadTest --warmup 2 --duration 7 --iterations 2 --parallel 2

Testing Track 2 - older versions

  • Example: To test 12.2.0
    • Update "@azure/storage-blob" version in package.json to 12.2.0
    • Add a new exception in common\config\rush\common-versions.json under allowedAlternativeVersions
      • "@azure/storage-blob": [..., "12.2.0"]
  • rush update (generates a new pnpm-lock file)
  • Navigate to sdk\storage\perf-tests\storage-blob
  • rush build -t perf-test-storage-blob
  • Add .env file as suggested in the readme
  • Run the tests as suggested by readme, example npm run perf-test:node -- StorageBlobDownloadTest --warmup 2 --duration 7 --iterations 2 --parallel 2

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Overall looks great, I like the move a lot. 👍

Left some small comments/suggestions.

common/tools/perf-tests-track-1-setup.js Outdated Show resolved Hide resolved
### Guide

1. Navigate to `sdk\storage\perf-tests\storage-blob-track-1`
2. Do `rush update`.
Copy link
Member

Choose a reason for hiding this comment

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

should one rebuild storage-blob itself as well? e.g. rush rebuild -t @azure/storage-blob ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is track-1(Version 10), we don't maintain it in our repo.
So, building storage-blob is not needed for track-1.

npm run setup in the next line, builds the test-utils-perfstress project, and installs it here.

(Also, track-1 perf tests are not maintained by rush, they are independent npm projects.)

sdk/storage/perf-tests/storage-blob-track-1/package.json Outdated Show resolved Hide resolved
{
"extends": "../../../../tsconfig.package",
"compilerOptions": {
"target": "es5",
Copy link
Member

Choose a reason for hiding this comment

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

target: es5 but lib: es5, es6, es7, esnext feels like a huge mismatch.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied the tsconfig that we have for storage-blob SDK.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like I need "lib": ["dom", "esnext"], removed the rest.

"outDir": "./dist-esm",
"lib": ["dom", "es5", "es6", "es7", "esnext"]
},
"compileOnSave": true,
Copy link
Member

Choose a reason for hiding this comment

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

does this work with rush?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same, copied the tsconfig for storage-blob SDK.

Copy link
Member Author

Choose a reason for hiding this comment

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

compileOnSave is working when tried editing TS files in VSCode.

rush.json Outdated
@@ -632,6 +632,11 @@
"projectFolder": "sdk/digitaltwins/digital-twins-core",
"versionPolicyName": "client"
},
{
"packageName": "@azure/perf-test-storage-blob",
Copy link
Member

Choose a reason for hiding this comment

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

If we're moving forward with the approach of using little micro-packages that are rush-managed, can we put the "perf-test" in the namespace part of the name? I'm just thinking out loud that it will be useful for scripting/bulk processing if these are @azure-perf-test/storage-blob than otherwise. It'll make it easiest to query package names for specific kinds of packages, since we will eventually have hundreds of packages as we add more services and more perf tests. Maybe we could rename dev-tool, eslint-plugin-azure-sdk, test-utils-recorder, and the other internal tools to use an @azure-tools namespace as well.

Just something to consider.

Copy link
Member Author

@HarshaNalluru HarshaNalluru Feb 18, 2021

Choose a reason for hiding this comment

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

Very good idea. I love @azure-perf-test/storage-blob.

Copy link
Member Author

Choose a reason for hiding this comment

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

A couple of things came up while making this change.
rush strips out the namespace name and saves it as this.
image
Not that it doesn't work, but something to consider. storage-blob.tgz and storage-blob-1.tgz can be confusing.

And I can no longer build storage-blob with rush build -t storage-blob, we must use rush build -t @azure/storage-blob to build storage-blob SDK which would break the Dev Exp.

Copy link
Member

Choose a reason for hiding this comment

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

@azure-tests/perf-storage-blob?

Copy link
Member Author

@HarshaNalluru HarshaNalluru Feb 18, 2021

Choose a reason for hiding this comment

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

I like that one too. Feels like a hack, but I don't have any objections.
Going with @azure-tests/perf-storage-blob.

(The constraint here is that the package name after stripping out the namespace has to be unique.)

@HarshaNalluru
Copy link
Member Author

/azp run js - storage-blob - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

Looks good!

sdk/storage/perf-tests/storage-blob-track-1/README.md Outdated Show resolved Hide resolved
rush.json Outdated
@@ -632,6 +632,11 @@
"projectFolder": "sdk/digitaltwins/digital-twins-core",
"versionPolicyName": "client"
},
{
"packageName": "@azure/perf-test-storage-blob",
Copy link
Member

Choose a reason for hiding this comment

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

@azure-tests/perf-storage-blob?

Copy link
Member

@EmmaZhu EmmaZhu left a comment

Choose a reason for hiding this comment

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

It looks much clearer to manage test packages in this way.
LGTM

@ghost
Copy link

ghost commented Feb 23, 2021

Hello @HarshaNalluru!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 91496d9 into Azure:master Feb 23, 2021
@HarshaNalluru HarshaNalluru deleted the harshan/perf-restructuring-proposal-2 branch February 23, 2021 20:30
ghost pushed a commit that referenced this pull request Mar 4, 2021
…talake perf tests to the latest model (#14075)

### Description
Makes changes to `storage-file-share` and `storage-file-datalake` based on #13770 and #14024


### Old
  - `sdk/storage/storage-file-share/test/perfstress/track-1/` - npm project (but imports the perf framework)
  - `sdk/storage/storage-file-share/test/perfstress/track-2/` - depends on src code like the regular tests

### New
  - `sdk/storage/perf-tests/storage-file-share-track-1/` - Independent npm project (but imports the perf framework)
  - `sdk/storage/perf-tests/storage-file-share/`              - New project maintained through "rush" (depends on storage-file-share on master)

The following walk-through for executing perf tests uses storage-file-share as an example. (Check #13740 for proposal 1 - old.)

## Testing Track 1
- `rush update`
- Navigate to `sdk\storage\perf-tests\storage-file-share-track-1`
- Run `npm run setup` (Builds the perf package and installs it for track-1 perf tests)
- Add .env file as suggested in the readme
- Run the tests as suggested by readme, example `npm run perf-test:node -- StorageFileShareDownloadTest --warmup 2 --duration 7 --iterations 2 --parallel 2`

## Testing Track 2 - unpublished(current master) 
- `rush update`
- Navigate to `sdk\storage\perf-tests\storage-file-share`
- `rush build -t perf-test-storage-file-share`
- Add .env file as suggested in the readme
- Run the tests as suggested by readme, example `npm run perf-test:node -- StorageFileShareDownloadTest --warmup 2 --duration 7 --iterations 2 --parallel 2`

## Testing Track 2 - older versions
-  Example: To test 12.2.0 
   - Update `"@azure/storage-file-share"` version in `package.json` to `12.2.0`
   - Add a new exception in `common\config\rush\common-versions.json` under `allowedAlternativeVersions`
      - `"@azure/storage-file-share": [..., "12.2.0"]`
- `rush update` (generates a new pnpm-lock file)
- Navigate to `sdk\storage\perf-tests\storage-file-share`
- `rush build -t perf-test-storage-file-share`
- Add .env file as suggested in the readme
- Run the tests as suggested by readme, example `npm run perf-test:node -- StorageFileShareDownloadTest --warmup 2 --duration 7 --iterations 2 --parallel 2`
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Apr 6, 2021
Communication cli directive config checkin (Azure#13770)

* communication-cli-directive-config-checkin

* typo

* typo
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants