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

[test-recorder] Support test context from vitest #28350

Merged
merged 8 commits into from
Jan 31, 2024

Conversation

jeremymeng
Copy link
Member

@jeremymeng jeremymeng commented Jan 23, 2024

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

   //...
  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 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) {
Copy link
Member

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?

Copy link
Member Author

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
Copy link
Member

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

Copy link
Member Author

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.

@mpodwysocki
Copy link
Member

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);
  });

Comment on lines 98 to 104
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(),
Copy link
Member

@HarshaNalluru HarshaNalluru Jan 23, 2024

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 :)

Copy link
Member

@HarshaNalluru HarshaNalluru 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 overall, nice to see the changes needed are rather relatively simple.

Copy link
Member

@HarshaNalluru HarshaNalluru left a 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
Copy link
Member

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?

Copy link
Member Author

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.

@jeremymeng jeremymeng merged commit a2a2d03 into Azure:main Jan 31, 2024
13 checks passed
@jeremymeng jeremymeng deleted the recorder/vitest-state branch January 31, 2024 03:44
jeremymeng added a commit to jeremymeng/azure-sdk-for-js that referenced this pull request Jan 31, 2024
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.
@jeremymeng jeremymeng mentioned this pull request Jan 31, 2024
jeremymeng added a commit that referenced this pull request Jan 31, 2024
PR #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 (#28350)"

This reverts commit a2a2d03.
jeremymeng added a commit to jeremymeng/azure-sdk-for-js that referenced this pull request Feb 22, 2024
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);
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants