-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[test-recorder] Support test context from vitest #28350
Conversation
Currently we just need suite title and test title to generate recording file names. `vitest` provides the info via `expect.getState().currentTestName`. This PR adds vitest support to recorder. Call site would look like ```ts import { assert, afterEach, beforeEach, describe, it, expect } from "vitest"; //... recorder = new Recorder(expect.getState()); ```
suiteTitle: string; | ||
testTitle: string; | ||
}; | ||
if (this.testContext instanceof MochaTest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we should trust instanceof
here since there could be multiple versions of Mocha... can we define a type guard that just looks at the object properties instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
constructor(private testContext?: Test | undefined) { | ||
constructor( | ||
private testContext?: | ||
| MochaTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't hard code to either Mocha or Vitest here as we can just encompass them with interfaces. See https://gist.github.com/mpodwysocki/c0afe1f92a16d839e4bc925cad647caf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to use your testInfo.ts. I updated types for vitest to account for the nested suites.
Why wouldn't we just use the context like this? beforeEach((ctx) => {
console.log("Context:", ctx.task.name);
console.log("Suite:", ctx.task.suite.name);
}); |
const parts = this.testContext.currentTestName.split(" > "); | ||
if (parts.length < 3) { | ||
throw new RecorderError(`Unable to determine the recording file path. Unexpected Vitest currentTestname`); | ||
} | ||
context = { | ||
suiteTitle: parts.slice(1, parts.length - 1).join("_").trim(), | ||
testTitle: parts[parts.length - 1].trim(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could pull this out and make a unit test out of it to simplify the constructor and for sanity :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, nice to see the changes needed are rather relatively simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍏
* @param test - The test to check. | ||
* @returns true if the given test is a Mocha Test. | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this because the method takes any
instead of unknown
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes switching to unknown
gets rid of it.
PR Azure#28350 somehow cause many packages to fail to build because they don't have a dev dependency on @types/mocha. Reverting for now. Revert "[test-recorder] Support test context from vitest (Azure#28350)" This reverts commit a2a2d03.
Currently we just need suite title and test title to generate recording file names. `vitest` provides the info via `context` of the callback function. This PR adds vitest support to recorder. Call site would look like ```ts //... beforeEach(async (context) => { recorder = new Recorder(context); ```
Currently we just need suite title and test title to generate recording file names.
vitest
provides the info viacontext
of the callback function.This PR adds vitest support to recorder. Call site would look like