Conversation
maorleger
left a comment
maorleger
left a comment
There was a problem hiding this comment.
THANK YOU for starting this! I have a few comments but honestly they are not blocking. Will finish my review after my next meeting
|
|
||
| constructor(private testContext?: Test | undefined) { | ||
| constructor(private testContext?: TestInfo) { | ||
| if (!this.testContext) { |
There was a problem hiding this comment.
👍 much prefer this style of coding. Does that also allow us to remove the if (this.testContext) call in line 109 since we validated it exists already?
Also, previous code only threw if this.testContext is falsy and we're in recording / playback mode. So should this check also skip throwing if we're in live mode?
| import { relativeRecordingsPath } from "./relativePathCalculator.js"; | ||
|
|
||
| export function sessionFilePath(testContext: Mocha.Test): string { | ||
| export interface TestContext { |
There was a problem hiding this comment.
Should this live under testInfo ?
| } catch (error: any) { | ||
| expect((error as RecorderError).name).to.equal("RecorderError"); | ||
| expect((error as RecorderError).message).to.equal( | ||
| assert.equal((error as RecorderError).name, "RecorderError"); |
There was a problem hiding this comment.
total nitpicking, and feel free to ignore since it's also existing code, but I don't believe this error as RecorderError gives us anything here (it's an any today, and as conversions aren't checked). Would be nice to do a minor cosmetic cleanup while we're here
| "compilerOptions": { | ||
| "outDir": "./dist-test/browser", | ||
| "rootDir": ".", | ||
| "skipLibCheck": true |
There was a problem hiding this comment.
Talk to me friendo, why do we need to skip lib check?
There was a problem hiding this comment.
Until we fix the whole browser testing stuff, it's just this way for now.
|
Closing in favor of #28667 |
3 participants
Packages impacted by this PR
Issues associated with this PR
Describe the problem that is addressed by this PR
Migrates the @azure-tools/test-recorder to tshy and ESM to support both vitest and Mocha
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists