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.

Sign up for GitHub

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

Upgrade to vitest2 #530

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Upgrade to vitest2 #530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4a5b12f
Upgrade to vitest2
ericanderson Jul 25, 2024
f789f73
Migrate foundry-sdk-generators e2e tests
ericanderson Jul 26, 2024
579b4a4
Restore __dirname for cjs
ericanderson Jul 31, 2024
acb7ee3
fix generate
ericanderson Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .changeset/lovely-kangaroos-think.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
---
---
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ packages/client/src/generatedNoCheck
packages/client.test.ontology/src/generatedNoCheck
packages/legacy-client/src/generatedNoCheck
packages/create-app.template.*/src/generatedNoCheck
packages/e2e.test.foundry-sdk-generator/src/generatedNoCheck

.log
pnpm-publish-summary.json
Expand Down
14 changes: 12 additions & 2 deletions .monorepolint.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const nonStandardPackages = [
"@osdk/e2e.sandbox.todoapp",
"@osdk/examples.*",
"@osdk/foundry-sdk-generator",
"@osdk/e2e.test.foundry-sdk-generator",
"@osdk/monorepo.*", // internal monorepo packages
"@osdk/shared.client", // hand written package that only exposes a symbol
"@osdk/tests.*",
Expand Down Expand Up @@ -241,7 +242,7 @@ function getTsconfigOptions(baseTsconfigPath, opts) {
* legacy: boolean,
* esmOnly?: boolean,
* customTsconfigExcludes?: string[],
* tsVersion?: "^5.5.2"|"^4.9.5",
* tsVersion?: typeof LATEST_TYPESCRIPT_DEP | "^4.9.5",
Copy link
Member Author

Choose a reason for hiding this comment

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

Just preventing future annoying behavior. Typescript doesn't actually check the .mjs files so everything was working here with any string but we were getting red in the IDE. This will at least prevent that mismatch.

* skipTsconfigReferences?: boolean
* singlePackageName?: string
* }} options
Expand Down Expand Up @@ -491,7 +492,16 @@ NOTE: DO NOT EDIT THIS README BY HAND. It is generated by monorepolint.
legacy: false,
tsVersion: LATEST_TYPESCRIPT_DEP,
customTsconfigExcludes: [
"./src/__e2e_tests__/**/**.test.ts",
"./src/generatedNoCheck/**/*",
],
}),
...standardPackageRules({
includePackages: ["@osdk/e2e.test.foundry-sdk-generator"],
ericanderson marked this conversation as resolved.
Show resolved Hide resolved
}, {
legacy: false,
esmOnly: true,
tsVersion: LATEST_TYPESCRIPT_DEP,
customTsconfigExcludes: [
"./src/generatedNoCheck/**/*",
],
}),
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
"turbo": "^2.0.9",
"typescript": "^5.5.3",
"typescript-eslint": "^7.17.0",
"vitest": "^1.6.0"
"vitest": "^2.0.4"
},
"pnpm": {
"overrides": {
Expand Down
28 changes: 11 additions & 17 deletions packages/client/src/object/Cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { promiseStateAsync as pStateAsync } from "p-state";
import type { Mock, MockInstance } from "vitest";
import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
import type { MinimalClient } from "../MinimalClientContext.js";
import type { AsyncClientCache } from "./Cache.js";
import type { AsyncClientCache, AsyncFactory } from "./Cache.js";
import { createAsyncClientCache, createClientCache } from "./Cache.js";

declare module "vitest" {
Expand Down Expand Up @@ -89,20 +89,14 @@ describe("AsyncCache", () => {
});

describe("race checks", () => {
let factoryFn: Mock<[MinimalClient, string], Promise<any>>;
let factoryFn: Mock<AsyncFactory<any, any>>;
Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, this is a way better type api for vitest (taking the typeof a function instead of an array of args and a return value). Alas, it required minor changes in this file

let cache: ReturnType<typeof createSpys>;
let inProgress: ReturnType<typeof createSpys>;
let asyncCache: AsyncClientCache<string, string>;
let asyncCacheSpies: {
[K in keyof typeof asyncCache]: MockInstance<
Parameters<typeof asyncCache[K]>,
ReturnType<typeof asyncCache[K]>
>;
[K in keyof typeof asyncCache]: MockInstance<typeof asyncCache[K]>;
};
let asyncSetSpy: MockInstance<
[client: MinimalClient, key: string, value: string | Promise<string>],
Promise<any>
>;
let asyncSetSpy: MockInstance<typeof asyncCache.set>;
let factoryDefers: DeferredPromise<string>[];
let getPromises: Promise<string>[];

Expand All @@ -114,7 +108,7 @@ describe("AsyncCache", () => {
let expectedPending: Record<string, number> = {};

beforeEach(async () => {
factoryFn = vi.fn<any, Promise<any>>();
factoryFn = vi.fn();
factoryFn.mockImplementation(() => {
const defer = pDefer<string>();
factoryDefers.push(defer);
Expand Down Expand Up @@ -274,10 +268,10 @@ describe("AsyncCache", () => {
i++
) {
if (asyncCacheSpies.get.mock.calls[i][1] === key) {
expect(asyncCacheSpies.get.mock.results[i].type).toBe(
"throw",
expect(asyncCacheSpies.get.mock.settledResults[i].type).toBe(
Copy link
Member Author

Choose a reason for hiding this comment

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

vitest in 1.x was very very strange wrt to how it handled functions that returned promises. In the old way, if the returned promise was resolved/rejected, the value itself was treated like the return value from the function. Otherwise it was treated as pending. Of course a function that returns a Promise could throw unrelated to that promise and you might want to test for that. So they changed the API to be far far more logical in terms of whats happening in the underlying system but that also broke all these carefully crafted tests.

settledResults is a lot like the former results except it only fills out AFTER the promises have been resolved/rejected. In almost all of our tests, this was a pure replacement but in some cases we had to change things.

"rejected",
);
expect(asyncCacheSpies.get.mock.results[i].value)
expect(asyncCacheSpies.get.mock.settledResults[i].value)
.toMatchInlineSnapshot(`[Error: aError]`);
}
}
Expand Down Expand Up @@ -352,7 +346,7 @@ describe("AsyncCache", () => {

function itReturnsForAsyncGet(results: any[]) {
it("returns for async get", () => {
expect(asyncCacheSpies.get.mock.results.map(a => a.value))
expect(asyncCacheSpies.get.mock.settledResults.map(a => a.value))
.toEqual(results);
});
}
Expand Down Expand Up @@ -396,8 +390,8 @@ describe("AsyncCache", () => {
]);

itReturnsForAsyncGet([
expect.any(Promise),
expect.any(Promise),
undefined,
Copy link
Member Author

Choose a reason for hiding this comment

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

Before we could get an object that appeared to be a Promise from .results when the object was result was pending. So these excpect.any(Promise) would technically work. However now they split the settledResults out and as those first two promises are still pending, the result is undefined in vitest.

undefined,
"bResult",
]);
});
Expand Down
15 changes: 13 additions & 2 deletions packages/client/src/object/Cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ interface ClientCache<K, V> {
}

/**
* @internal
* A simple async cache that can be used to store values for a given client.
*/
export interface AsyncClientCache<K, V> {
Expand All @@ -46,16 +47,19 @@ export interface AsyncClientCache<K, V> {
) => Promise<V>;
}

type Factory<K, V> = (client: MinimalClient, key: K) => V;
/** @internal */
export type Factory<K, V> = (client: MinimalClient, key: K) => V;

/**
* @internal
* Create a new cache without a factory function.
*/
export function createClientCache<K, V extends {}>(): ClientCache<
K,
V | undefined
>;
/**
* @internal
* Create a new cache with a factory function.
* @param fn A factory function that will be used to create the value if it does not exist in the cache.
*/
Expand Down Expand Up @@ -101,13 +105,20 @@ export function createClientCache<K, V extends {}>(
return { get, set, remove } as ClientCache<K, V>;
}

/** @internal */
export type AsyncFactory<K, V extends {}> = (
client: MinimalClient,
key: K,
) => Promise<V>;

/**
* @internal
* Create a new cache with an async factory function.
* @param fn A factory function that will be used to create the value if it does not exist in the cache.
* @returns
*/
export function createAsyncClientCache<K, V extends {}>(
fn: (client: MinimalClient, key: K) => Promise<V>,
fn: AsyncFactory<K, V>,
createCacheLocal: typeof createClientCache = createClientCache,
): AsyncClientCache<K, V> {
const cache = createCacheLocal<K, V>();
Expand Down
2 changes: 1 addition & 1 deletion packages/client/src/objectSet/ObjectSetListenerWebsocket.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ function createMockWebSocketConstructor(
eventEmitter.removeEventListener.bind(eventEmitter),
) as any,

send: vi.fn((a, _b) => {
send: vi.fn((a, _b: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why'd you need to add a type for _b?

Copy link
Member Author

Choose a reason for hiding this comment

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

in the old way of the type being args and return value, there was only one way to resolve the function. But now that its typeof foo and there can be overloads, it can't infer properly what this value is because in one case its missing and in another it was defined. This was fine before, we didnt use it and i just told it about the defined value in the type but we don't have that any more.

logger.debug(
{ message: JSON.parse(a.toString()), webSocketInst },
"send() called",
Expand Down
82 changes: 69 additions & 13 deletions ...-generator/src/__e2e_tests__/testSetup.ts → ...dry-sdk-generator/generateMockOntology.js
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2023 Palantir Technologies, Inc. All rights reserved.
* Copyright 2024 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -14,21 +14,31 @@
* limitations under the License.
*/

// @ts-check
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 file is the old test setup file from foundry-sdk-generator except now its run as a script. I've taken care to try to keep as many lines as they were to keep the diff down. Thus you'll see things like setup and teardown still being used below so that indentation stays the same.

import { __testSeamOnly_NotSemverStable__GeneratePackageCommand as GeneratePackageCommand } from "@osdk/foundry-sdk-generator";
import { apiServer } from "@osdk/shared.test";
import { rm } from "fs/promises";
import { join } from "path";
import { GeneratePackageCommand } from "../generate/index.js";
import * as fs from "node:fs/promises";
import * as path from "node:path";
import { fileURLToPath } from "node:url";

const dir = `${__dirname}/../generatedNoCheck/`;
export async function setup() {
const dir = path.join(
Copy link
Member Author

Choose a reason for hiding this comment

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

ESM doesn't have __dirname. This worked in vitest 1 because in reality vitest was converting all code to CJS before executing it and CJS has __dirname. However the tests are actually being run as ESM now and thus this breaks.

path.dirname(fileURLToPath(import.meta.url)),
"src",
"generatedNoCheck",
);
async function setup() {
apiServer.listen();

try {
await rm(join(dir, "@test-app"), { recursive: true });
Copy link
Member Author

Choose a reason for hiding this comment

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

In the old code, this could throw causing the second rm to not run.

await rm(join(dir, "@test-app2"), { recursive: true });
} catch (e) {
// Only needed for regenerations
}
const testAppDir = path.join(dir, "@test-app");
const testApp2Dir = path.join(dir, "@test-app2");

await rmRf(testAppDir);
await rmRf(testApp2Dir);

await safeStat(testAppDir, "should not exist");
await safeStat(testApp2Dir, "should not exist");
Copy link
Member Author

Choose a reason for hiding this comment

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

After upgrading to vitest 2, on initial runs of this test suite, everything would fail due to not being able to find the generated code. And then later when I was testing to find out what was happening I realized that if you bailed on setup early (after deleting) that it ALSO was failing. So I added some safety checks to be sure that things were being deleted (they were).

Turns out that in vitest 1, the global setup would run before any forks for the test runners would happen but in vitest 2, the forks happen right away. There also seemed to be some caching of the filesystem on initial load such that no matter what, on the first run, even though I could confirm we fully regenerated prior to the test running, the test would still think those files didn't exist.

I figured there was no harm in keeping the extra sanity checks after I chased the behavior down, so it remains.


await fs.mkdir(dir, { recursive: true });

const generatePackageCommand = new GeneratePackageCommand();
await generatePackageCommand.handler({
Expand Down Expand Up @@ -69,6 +79,8 @@ export async function setup() {
$0: "",
});

await safeStat(testAppDir, "should exist");

await generatePackageCommand.handler({
packageName: "@test-app2/osdk",
packageVersion: "0.0.1",
Expand Down Expand Up @@ -107,10 +119,54 @@ export async function setup() {
_: [],
$0: "",
});

await safeStat(testApp2Dir, "should exist");
}

export async function teardown() {
// eslint-disable-next-line no-console
console.log("Test teardown: stopping API server");
console.log("teardown: stopping API server");
apiServer.close();
}

await setup();
await teardown();

/**
* @param {string} testAppDir
*/
async function rmRf(testAppDir) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Split the deletion out so its more readable up above and so that the first failure doesn't cause the be ignored.

try {
await fs.rm(testAppDir, { recursive: true });
} catch (e) {
// eslint-disable-next-line no-console
console.debug("rm error", e);
// Only needed for regenerations
}
}

/**
* @param {string} filePath
* @param {"should exist" | "should not exist"} type
* @returns
*/
async function safeStat(filePath, type) {
Copy link
Member Author

Choose a reason for hiding this comment

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

fs.stat throws if the file does not exist, which i generally don't care for as it makes ones code far more difficult to read, especially when you expect to be able to test for the non-existence of something. This helper both logs out whats going on as well as acting as an assert to clean up the rest of the codebase

try {
const ret = await fs.stat(filePath);
if (type !== "should exist") {
throw new Error(`Expected ${filePath} to not exist`);
}

// eslint-disable-next-line no-console
console.log(`safeStat: ${filePath} exists`);
return ret;
} catch (e) {
if (type === "should exist") {
throw new Error(`Expected ${filePath} to exist`);
}

// eslint-disable-next-line no-console
console.log(`safeStat: ${filePath} does not exist`);
return undefined;
}
}
11 changes: 6 additions & 5 deletions packages/e2e.test.foundry-sdk-generator/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,34 +9,35 @@
},
"exports": {
".": {
"require": "./build/cjs/index.cjs",
"browser": "./build/browser/index.js",
"import": "./build/esm/index.js"
},
"./*": {
"require": "./build/cjs/public/*.cjs",
"browser": "./build/browser/public/*.js",
"import": "./build/esm/public/*.js"
}
},
"scripts": {
"check-attw": "monorepo.tool.attw both",
"check-attw": "monorepo.tool.attw esm",
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 was a both before but didn't need to be. This simplifies logic and reduces build steps

"check-spelling": "cspell --quiet .",
"clean": "rm -rf lib dist types build tsconfig.tsbuildinfo",
"codegen": "node ./generateMockOntology.js",
"fix-lint": "eslint . --fix && dprint fmt --config $(find-up dprint.json)",
"lint": "eslint . && dprint check --config $(find-up dprint.json)",
"test": "vitest run --pool=forks",
"transpile": "monorepo.tool.transpile",
"typecheck": "monorepo.tool.typecheck both"
"typecheck": "monorepo.tool.typecheck esm"
},
"dependencies": {
"@osdk/foundry-sdk-generator": "workspace:~",
"execa": "^9.3.0"
},
"devDependencies": {
"@osdk/legacy-client": "workspace:~",
Copy link
Member Author

Choose a reason for hiding this comment

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

Because we execute some of the generated code we need to have access to its dependencies in this project.

"@osdk/monorepo.api-extractor": "workspace:~",
"@osdk/monorepo.tsconfig": "workspace:~",
"@osdk/monorepo.tsup": "workspace:~",
"@osdk/shared.test": "workspace:~",
"typescript": "^5.5.4"
},
"publishConfig": {
Expand All @@ -53,6 +54,6 @@
],
"main": "./build/cjs/index.cjs",
"module": "./build/esm/index.js",
"types": "./build/cjs/index.d.cts",
"types": "./build/esm/index.d.ts",
"type": "module"
}
7 changes: 3 additions & 4 deletions ...c/generate/GeneratePackageCommand.test.ts → ...erator/src/GeneratePackageCommand.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,14 @@
import { existsSync } from "node:fs";
import * as fs from "node:fs/promises";
import * as path from "node:path";
import { fileURLToPath } from "node:url";
import { describe, expect, it } from "vitest";
import { GeneratePackageCommand } from "./GeneratePackageCommand.js";

describe(GeneratePackageCommand, () => {
describe("Generate Package Command", () => {
// ensure that we do not break backcompat by retaining our scripts export that links to the bundled types and esm output
it("has a public scripts export", async () => {
const generatedPath = path.join(
__dirname,
"..",
path.dirname(fileURLToPath(import.meta.url)),
"generatedNoCheck",
"@test-app",
"osdk",
Expand Down
0 ...nerator/src/__e2e_tests__/actions.test.ts → ...nerator/src/legacy-client/actions.test.ts
View file
Open in desktop
File renamed without changes.
0 ...or/src/__e2e_tests__/aggregations.test.ts → ...or/src/legacy-client/aggregations.test.ts
View file
Open in desktop
File renamed without changes.
0 ...or/src/__e2e_tests__/attachements.test.ts → ...or/src/legacy-client/attachements.test.ts
View file
Open in desktop
File renamed without changes.
0 ...dk-generator/src/__e2e_tests__/consola.ts → ...dk-generator/src/legacy-client/consola.ts
View file
Open in desktop
File renamed without changes.
Loading