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

chore: uvu -> vitest for create-svelte tests #9910

Merged
merged 11 commits into from
May 16, 2023
4 changes: 2 additions & 2 deletions packages/create-svelte/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@
"sucrase": "^3.29.0",
"svelte": "^3.56.0",
"tiny-glob": "^0.2.9",
"uvu": "^0.5.6"
"vitest": "^0.31.0"
},
"scripts": {
"build": "node scripts/build-templates",
"test": "pnpm build && uvu test",
"test": "pnpm build && vitest run",
"check": "tsc",
"lint": "prettier --check . --config ../../.prettierrc --ignore-path ../../.gitignore --ignore-path .gitignore --plugin prettier-plugin-svelte --plugin-search-dir=.",
"format": "pnpm lint --write",
Expand Down
110 changes: 71 additions & 39 deletions packages/create-svelte/test/check.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { exec, execSync } from 'node:child_process';
import fs from 'node:fs';
import { execSync } from 'node:child_process';
import path from 'node:path';
import { test } from 'uvu';
import * as assert from 'uvu/assert';
import { create } from '../index.js';
import { fileURLToPath } from 'node:url';
import { promisify } from 'node:util';
import glob from 'tiny-glob';
import { beforeAll, describe, test } from 'vitest';
import { create } from '../index.js';

/** Resolve the given path relative to the current file */
const resolve_path = (path) => fileURLToPath(new URL(path, import.meta.url));
Expand Down Expand Up @@ -47,14 +47,56 @@ const workspace = {
pnpm: { overrides },
devDependencies: overrides
};

fs.writeFileSync(
path.join(test_workspace_dir, 'package.json'),
JSON.stringify(workspace, null, '\t')
);

fs.writeFileSync(path.join(test_workspace_dir, 'pnpm-workspace.yaml'), 'packages:\n - ./*\n');

for (const template of fs.readdirSync('templates')) {
if (template[0] === '.') continue;
const exec_async = promisify(exec);

beforeAll(
() =>
exec_async(`pnpm install --no-frozen-lockfile`, {
cwd: test_workspace_dir
}),
60000
);

function patch_package_json(pkg) {
Copy link
Member

Choose a reason for hiding this comment

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

it might be nice to pass in overrides vs depending on a global variable. would also need to tweak where it's being called then

Suggested change
function patch_package_json(pkg) {
function patch_package_json(pkg, overrides) {

Copy link
Member

Choose a reason for hiding this comment

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

but they are global, or rather shared by everything in the module. not sure what this would achieve — overrides would still be defined/populated at the module level, we'd just be replacing a closed-over value with an identical parameter, no?

Copy link
Member

@benmccann benmccann May 16, 2023

Choose a reason for hiding this comment

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

yes. I just think it's a bit cleaner to avoid depending on global values - e.g. if you wanted to have a unit test for the method it's easier that way. no functional difference though and just a matter of opinion

Object.entries(overrides).forEach(([key, value]) => {
if (pkg.devDependencies?.[key]) {
pkg.devDependencies[key] = value;
}

if (pkg.dependencies?.[key]) {
pkg.dependencies[key] = value;
}

if (!pkg.pnpm) {
pkg.pnpm = {};
}

if (!pkg.pnpm.overrides) {
pkg.pnpm.overrides = {};
}

pkg.pnpm.overrides = { ...pkg.pnpm.overrides, ...overrides };
});
pkg.private = true;
}

/**
* Tests in different templates can be run concurrently for a nice speedup locally, but tests within a template must be run sequentially.
* It'd be better to group tests by template, but vitest doesn't support that yet.
* @type {Map<string, [string, () => import('node:child_process').PromiseWithChild][]>}
*/
const script_test_map = new Map();

function test_template(template) {
if (template[0] === '.') return;

for (const types of ['checkjs', 'typescript']) {
const cwd = path.join(test_workspace_dir, `${template}-${types}`);
Expand All @@ -68,49 +110,39 @@ for (const template of fs.readdirSync('templates')) {
eslint: true,
playwright: false
});

const pkg = JSON.parse(fs.readFileSync(path.join(cwd, 'package.json'), 'utf-8'));
Object.entries(overrides).forEach(([key, value]) => {
if (pkg.devDependencies?.[key]) {
pkg.devDependencies[key] = value;
}
if (pkg.dependencies?.[key]) {
pkg.dependencies[key] = value;
}
if (!pkg.pnpm) {
pkg.pnpm = {};
}
if (!pkg.pnpm.overrides) {
pkg.pnpm.overrides = {};
}
pkg.pnpm.overrides = { ...pkg.pnpm.overrides, ...overrides };
});
pkg.private = true;
patch_package_json(pkg);

fs.writeFileSync(path.join(cwd, 'package.json'), JSON.stringify(pkg, null, '\t') + '\n');

// run provided scripts that are non-blocking. All of them should exit with 0
// package script requires lib dir
const scripts_to_test = ['sync', 'format', 'lint', 'check', 'build'];
// TODO: lint should run before format
let scripts_to_test = ['sync', 'format', 'lint', 'check', 'build'];
if (fs.existsSync(path.join(cwd, 'src', 'lib'))) {
scripts_to_test.push('package');
}
scripts_to_test = scripts_to_test.filter((s) => !!pkg.scripts[s]);

for (const script of scripts_to_test.filter((s) => !!pkg.scripts[s])) {
test(`${template}-${types}: ${script}`, () => {
try {
execSync(`pnpm ${script}`, { cwd, stdio: 'pipe' });
} catch (e) {
assert.unreachable(
`script: ${script} failed\n` +
`---\nstdout:\n${e.stdout}\n` +
`---\nstderr:\n${e.stderr}`
);
}
});
for (const script of scripts_to_test) {
const tests = script_test_map.get(script) ?? [];
tests.push([`${template}-${types}`, () => exec_async(`pnpm ${script}`, { cwd })]);
script_test_map.set(script, tests);
}
}
}

console.log('installing dependencies...');
execSync('pnpm install --no-frozen-lockfile', { cwd: test_workspace_dir, stdio: 'ignore' });
console.log('done installing dependencies');
test.run();
fs.readdirSync('templates').map((template) => test_template(template));

for (const [script, tests] of script_test_map) {
describe.concurrent(
script,
() => {
for (const [name, task] of tests) {
test(name, task);
}
},
{ timeout: 60000 }
);
}
5 changes: 5 additions & 0 deletions packages/create-svelte/vitest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { defineConfig } from 'vitest/config';

export default defineConfig({
test: { dir: './test', include: ['*.js'] }
});
6 changes: 3 additions & 3 deletions packages/kit/src/runtime/server/cookie.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,10 @@ test('warns if cookie exceeds 4,129 bytes', () => {
const error = /** @type {Error} */ (e);

assert.equal(error.message, `Cookie "a" is too large, and will be discarded by the browser`);
} finally {
// @ts-expect-error
globalThis.__SVELTEKIT_DEV__ = false;
Copy link
Member

Choose a reason for hiding this comment

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

In kit.vitest.config.js, I put:

		isolate: false,
		singleThread: true,

I did that to speed up the tests, but I'm realizing if we're modifying global variables like this then that's probably not safe to do, so we should probably remove them

Copy link
Member

Choose a reason for hiding this comment

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

this has its own vitest.config.js which doesn't have those settings, so it should be fine i think?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it does have its own vitest.config.js? This file is located in the kit project unlike the others

Copy link
Member

Choose a reason for hiding this comment

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

ah right. can we mark a single file as requiring isolation? probably should discuss this in a separate PR

Copy link
Member

Choose a reason for hiding this comment

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

csp.spec.js does it too, so we'd need to do it to more than one file. Those flags only save 1-2s, so maybe it's not worth using them

}

// @ts-expect-error
globalThis.__SVELTEKIT_DEV__ = false;
});

test('get all cookies from header and set calls', () => {
Expand Down
6 changes: 3 additions & 3 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.