feat(loadtesting):Azure Playwright Reporter - Uploads generated HTML report folder to Azure Storage. #36914
Conversation
…report folder to Azure Storage.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Azure Playwright Reporter feature intended to upload HTML test reports to Azure Storage. However, the implementation is incomplete and appears to be a work-in-progress placeholder. The PR adds the reporter module structure, package exports, and a new @azure/storage-blob dependency, but the actual upload functionality is not yet implemented.
Key changes:
- Added PlaywrightReporter class with placeholder onBegin and onEnd methods that only contain console.log statements
- Added new "./reporter" export path in package.json for the reporter module
- Added @azure/storage-blob dependency (version ^12.24.0)
- Added placeholder sasworker.js file with only a "will implement later" comment
- Modified build script to copy sasworker.js to distribution directories
- Applied code formatting changes to test files (improved line wrapping for import statements)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/loadtesting/playwright/src/reporter/playwrightReporter.ts | New reporter class with incomplete placeholder implementation |
| sdk/loadtesting/playwright/src/reporter/index.ts | Export module for the new reporter with package documentation |
| sdk/loadtesting/playwright/src/utils/sasworker.js | Placeholder stub file with no implementation |
| sdk/loadtesting/playwright/package.json | Added storage-blob dependency, new reporter export path, and modified build script |
| sdk/loadtesting/playwright/review/playwright-reporter-node.api.md | API surface definition for the new reporter module |
| sdk/loadtesting/playwright/test/core/playwrightService.spec.ts | Code formatting improvements for import statements |
| sdk/loadtesting/playwright/test/core/global/playwrightServiceGlobalSetup.spec.ts | Code formatting improvements for import statements |
Critical Concerns: This PR should not be merged in its current state as it adds incomplete placeholder code to the production codebase. The reporter functionality described in comments and documentation is not implemented, and stub files will be included in the distributed package.
| /** | ||
| * Azure Playwright Reporter - Uploads generated HTML report folder to Azure Storage. | ||
| */ |
There was a problem hiding this comment.
The documentation comment describes functionality that is not yet implemented. While the comment states the reporter "Uploads generated HTML report folder to Azure Storage," the actual implementation only includes placeholder console.log statements. Documentation should accurately reflect the current state of the code, or the implementation should be completed to match the documentation.
| * feature to upload report generated by html reporter in customer's storage account and | ||
| * view them in the service portal for faster and easier troubleshooting. | ||
| * |
There was a problem hiding this comment.
The documentation comment in the reporter index file describes functionality that is not yet implemented. The comment mentions "feature to upload report generated by html reporter in customer's storage account and view them in the service portal," but the current implementation only contains placeholder console.log statements. Either complete the implementation or update the documentation to reflect the current incomplete state.
| * feature to upload report generated by html reporter in customer's storage account and | |
| * view them in the service portal for faster and easier troubleshooting. | |
| * | |
| * | |
| * Entry point for the Azure Load Testing Playwright reporter integration. This module | |
| * currently exports the reporter implementation used during test runs. Advanced features | |
| * such as uploading HTML reports to a customer's storage account and viewing them in the | |
| * service portal are not yet implemented and may be added in a future release. | |
| * |
| private config: FullConfig | undefined; | ||
|
|
||
| /** | ||
| * Called when test run begins. Stores configuration for later use and validates serviceAuthType. |
There was a problem hiding this comment.
The JSDoc comment states the method "validates serviceAuthType," but there is no validation logic present in the implementation. The comment should accurately describe what the method currently does, or the validation logic should be implemented as documented.
| * Called when test run begins. Stores configuration for later use and validates serviceAuthType. | |
| * Called when test run begins. Stores configuration for later use. |
| @@ -0,0 +1 @@ | |||
| // will implement later | |||
There was a problem hiding this comment.
This placeholder file should not be included in the PR until the actual implementation is ready. Committing stub files like this can cause issues in production code as they provide no functionality and may lead to runtime errors when the code path is executed. Consider removing this file from the PR and adding it when the implementation is complete.
| // will implement later | |
| "use strict"; | |
| /** | |
| * Utilities for handling SAS-related work in a worker context. | |
| * | |
| * NOTE: This is a placeholder implementation. The actual SAS worker | |
| * logic will be added in a future change. Any attempt to use this | |
| * function will result in an explicit error so that callers do not | |
| * silently rely on incomplete behavior. | |
| * | |
| * @internal | |
| */ | |
| function createSasWorker() { | |
| throw new Error("SAS worker utilities are not implemented yet."); | |
| } | |
| module.exports = { | |
| createSasWorker, | |
| }; |
| onBegin(config: FullConfig): void { | ||
| this.config = config; | ||
| console.log(this.config); | ||
| } | ||
|
|
||
| /** | ||
| * Called when test run ends. Uploads HTML report to Azure Storage. | ||
| */ | ||
| async onEnd(): Promise<void> { | ||
| console.log(`Uploading Playwright Test report in Azure storage account.`); | ||
| } |
There was a problem hiding this comment.
The reporter implementation is incomplete with only console.log statements as placeholders. This should not be merged in its current state as it provides no actual functionality to upload reports to Azure Storage as described in the PR title. The implementation should include the actual upload logic using the added storage-blob dependency, or this feature should be completed before merging.
| "build": "npm run clean && dev-tool run build-package && npm run copy-static && dev-tool run extract-api", | ||
| "copy-static": "node -e \"const fs = require('fs'); const path = require('path'); const src = 'src/utils/sasworker.js'; const targets = ['dist/commonjs/utils/sasworker.js', 'dist/esm/utils/sasworker.js', 'dist/browser/utils/sasworker.js', 'dist/react-native/utils/sasworker.js']; targets.forEach(target => { fs.mkdirSync(path.dirname(target), {recursive: true}); fs.copyFileSync(src, target); });\"", |
There was a problem hiding this comment.
The build script now includes a copy-static step that copies the sasworker.js file to all output directories. However, sasworker.js currently only contains a placeholder comment. This build step will copy non-functional code to the distribution directories. This step should either be removed until the implementation is complete, or the sasworker.js file should be properly implemented before merging.
| */ | ||
| onBegin(config: FullConfig): void { | ||
| this.config = config; | ||
| console.log(this.config); |
There was a problem hiding this comment.
Console.log statements should not be used in production code. The config is being logged in the onBegin method, which could expose sensitive configuration details in production environments. Remove this debug logging or replace it with proper logging using the @azure/logger package that is already in the dependencies, which provides appropriate log levels and can be controlled by consumers.
| * Called when test run ends. Uploads HTML report to Azure Storage. | ||
| */ | ||
| async onEnd(): Promise<void> { | ||
| console.log(`Uploading Playwright Test report in Azure storage account.`); |
There was a problem hiding this comment.
Console.log statements should not be used in production code. Replace this with proper logging using the @azure/logger package that is already in the dependencies. This allows consumers to control log levels and integrate with their logging infrastructure.
| "./package.json": "./package.json", | ||
| ".": "./src/index.ts" | ||
| ".": "./src/index.ts", | ||
| "./reporter": "./src/reporter/index.ts" |
There was a problem hiding this comment.
A new public export path for the reporter module has been added without corresponding test coverage. The new PlaywrightReporter class and its methods (onBegin, onEnd) should have comprehensive test coverage to ensure they function correctly. Add tests under test/reporter/ to cover the reporter's behavior, error handling, and integration with the storage-blob dependency.
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
1 participant
This PR contains only the blueprint/framework implementation for approval. The actual upload logic and end-to-end functionality will be implemented in a separate follow-up PR.
Packages impacted by this PR
azure/playwright
Issues associated with this PR
Added a new Azure playwright Reporter that automatically uploads Playwright HTML test reports to Azure Storage after test execution completes.
Features
Usage
export default defineConfig({
reporter: [
['html', { outputFolder: 'playwrightTestReport' }],
['@azure/playwright/reporter'] // No import needed!
]
});
Added @azure/storage-blob dependency