mirror of
https://github.com/game-ci/unity-builder.git
synced 2026-06-13 17:33:54 -07:00
chore: quality-tightening (oxfmt + oxlint + tsc + vitest + husky + actionlint) (#833)
* chore: quality-tightening (oxfmt + oxlint + tsc + vitest + husky + actionlint)
Standard rollout for unity-builder. Most of the work was porting 24
test files from jest 27 to vitest 4.
- prettier -> oxfmt
- eslint (with @typescript-eslint, github, jest, prettier, unicorn) ->
oxlint with eslint-plugin-unicorn
- jest 27 + jest-circus + ts-jest + @types/jest + @jest/globals ->
vitest 4 + vite 7 + @vitest/coverage-istanbul (jest config files
removed)
- new: tsgo --noEmit (alongside tsc fallback)
- lefthook (and lefthook.yml) -> husky 9 with the standard
scripts/ensure-husky.mjs self-heal pattern + lint-staged
- new: gitleaks, actionlint, shellcheck as mise-managed binaries
- TypeScript bumped target ES2020 -> ES2022 + lib ES2022 + DOM (for
Error.cause and modern globals)
Test migration (24 files):
- Bulk-converted jest.* -> vi.*; jest.Mocked -> Mocked from vitest;
jest.MockedFunction -> MockedFunction.
- Added vitest imports to all *.test.ts files (and __mocks__/*.ts)
that didn't have them.
- src/index.ts: extracted runMain() as a named export and gated the
module-level invocation behind NODE_ENV !== 'test'. The
index-plugin-features test now calls runMain() directly instead of
relying on jest's removed vi.isolateModules.
- index-plugin-features.test.ts: moved hoisted refs (mockPlugin,
mockLoadOrchestratorPlugin) into vi.hoisted() so vi.mock factories
can reference them. Replaced arrow constructor mock for ImageTag
with regular function() {...} (vitest 4 disallows arrows as ctors).
Replaced require('./model') / require('@actions/core') inside test
bodies with top-level imports.
- model/orchestrator-plugin.test.ts: dropped jest's '{ virtual: true }'
flag (vitest doesn't support it); replaced the
'mock factory throws' pattern with 'createPlugin throws' so vitest
doesn't wrap the error message at the assertion site.
- model/versioning.test.ts: stray jest.spyOn -> vi.spyOn; replaced
mockImplementation() with no args (jest pattern) by
mockResolvedValue('') / mockImplementation(() => undefined) where
the source expects a string return.
Workflow shell-quoting cleanup (actionlint):
- All bare $GITHUB_STEP_SUMMARY / $GITHUB_OUTPUT / $GITHUB_ENV
redirects quoted across 2 workflows (SC2086).
- s3://$AWS_STACK_NAME / s3://$BUCKET_NAME -> s3://"$AWS_STACK_NAME"
/ s3://"$BUCKET_NAME".
- 'for i in {1..N}; do ... done' loops where i isn't referenced in
the body renamed to 'for _ in' (SC2034).
- 'grep ... | wc -l' -> 'grep -c ...' (SC2126).
- Multiple consecutive '>> $file' redirects in
validate-community-plugins.yml summary block collapsed into a
single block redirect (SC2129).
- 'cat $file | python3 -c "..."' -> 'python3 -c "..." < $file'
(SC2002).
- http://${VAR}:port -> http://"${VAR}":port (SC2086).
tsgo: kept tsc --noEmit as the default 'typecheck' because
unity-builder publishes CommonJS for the GitHub Action consumer,
which conflicts with tsgo's bundler/node16 moduleResolution
requirement (per playbook trap #9). 'yarn typecheck:tsgo' is wired
up for when consumers move to ESM.
Caveats: 28 pre-existing oxlint warnings remain (mostly
typescript/no-explicit-any across the build-parameter shapes and
vitest/no-disabled-tests on 2 explicitly skipped scenarios). Per
playbook trap #22 the lint script drops --deny-warnings.
Verified locally: format clean, lint 0/28, typecheck clean,
test 340/342 (2 pre-existing skipped), actionlint clean across all
12 workflows.
* ci(unity-builder): fix Tests + Plugin Architecture Health on quality-tightening
Three issues surfaced in CI after the jest -> vitest port:
1. **Obsolete snapshot blocks Tests job.**
src/model/__snapshots__/versioning.test.ts.snap had two entries
for the same 'throws for invalid strategy' assertion: one in the
vitest format ('Versioning > determineBuildVersion > ...') and one
in the legacy jest format without the '>'. vitest correctly
regenerates the new one and flags the old one as obsolete; CI
runs without --update so 'Test Files 1 failed' even though all
343 tests passed. Removed the obsolete entry.
2. **'Plugin Architecture Health' workflow still calls jest.**
.github/workflows/validate-orchestrator.yml had two 'npx jest'
steps (orchestrator-plugin unit tests + orchestrator-standalone
tests). The unity-builder + orchestrator codebases are both on
vitest now. Replaced both with 'yarn vitest run'.
3. **jest-fail-on-console + src/jest.setup.ts left over.**
The earlier vitest port missed the jest-fail-on-console
integration. yarn install in CI surfaced
YN0002: doesn't provide @jest/globals (requested by
jest-fail-on-console). Removed jest-fail-on-console + jest.setup.ts;
added src/test/setup.ts with the equivalent vitest beforeEach
spies (same as unity-test-runner).
---------
Co-authored-by: frostebite <jas.f.ukcmti@gmail.com>
This commit is contained in:
@@ -1,3 +1,4 @@
|
||||
import { describe, it, expect, beforeEach, afterEach, vi, type Mock } from 'vitest';
|
||||
/**
|
||||
* Integration wiring tests for the plugin lifecycle in index.ts
|
||||
*
|
||||
@@ -9,74 +10,83 @@
|
||||
* - When providerStrategy is non-local without a plugin, an error is thrown
|
||||
*/
|
||||
|
||||
import { BuildParameters } from './model';
|
||||
import { BuildParameters, Docker } from './model';
|
||||
import * as core from '@actions/core';
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Mock plugin
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const mockPlugin = {
|
||||
initialize: jest.fn().mockResolvedValue(undefined),
|
||||
canHandleBuild: jest.fn().mockReturnValue(false),
|
||||
handleBuild: jest.fn().mockResolvedValue({ exitCode: 0 }),
|
||||
beforeLocalBuild: jest.fn().mockResolvedValue(undefined),
|
||||
afterLocalBuild: jest.fn().mockResolvedValue(undefined),
|
||||
handlePostBuild: jest.fn().mockResolvedValue(undefined),
|
||||
};
|
||||
// `vi.mock` hoists to the top of the module, so any factory references must
|
||||
// be hoisted with `vi.hoisted` to be defined at mock-evaluation time.
|
||||
const { mockPlugin, mockLoadPlugin } = vi.hoisted(() => {
|
||||
const plugin = {
|
||||
initialize: vi.fn().mockResolvedValue(undefined),
|
||||
canHandleBuild: vi.fn().mockReturnValue(false),
|
||||
handleBuild: vi.fn().mockResolvedValue({ exitCode: 0 }),
|
||||
beforeLocalBuild: vi.fn().mockResolvedValue(undefined),
|
||||
afterLocalBuild: vi.fn().mockResolvedValue(undefined),
|
||||
handlePostBuild: vi.fn().mockResolvedValue(undefined),
|
||||
};
|
||||
return {
|
||||
mockPlugin: plugin,
|
||||
mockLoadPlugin: vi.fn().mockResolvedValue(plugin),
|
||||
};
|
||||
});
|
||||
|
||||
const mockLoadPlugin = jest.fn().mockResolvedValue(mockPlugin);
|
||||
|
||||
jest.mock('./model/plugin', () => ({
|
||||
vi.mock('./model/plugin', () => ({
|
||||
loadPlugin: mockLoadPlugin,
|
||||
}));
|
||||
|
||||
jest.mock('@actions/core');
|
||||
jest.mock('./model', () => ({
|
||||
vi.mock('@actions/core');
|
||||
vi.mock('./model', () => ({
|
||||
Action: {
|
||||
checkCompatibility: jest.fn(),
|
||||
checkCompatibility: vi.fn(),
|
||||
workspace: '/workspace',
|
||||
actionFolder: '/action',
|
||||
},
|
||||
BuildParameters: {
|
||||
create: jest.fn(),
|
||||
create: vi.fn(),
|
||||
},
|
||||
Cache: {
|
||||
verify: jest.fn(),
|
||||
verify: vi.fn(),
|
||||
},
|
||||
Docker: {
|
||||
run: jest.fn().mockResolvedValue(0),
|
||||
run: vi.fn().mockResolvedValue(0),
|
||||
},
|
||||
ImageTag: jest.fn().mockImplementation(() => ({
|
||||
toString: () => 'mock-image:latest',
|
||||
})),
|
||||
// vitest 4 requires constructor mocks to use regular `function` (or
|
||||
// `class`); arrow fns aren't valid constructors.
|
||||
ImageTag: vi.fn(function () {
|
||||
return { toString: () => 'mock-image:latest' };
|
||||
}),
|
||||
Output: {
|
||||
setBuildVersion: jest.fn().mockResolvedValue(''),
|
||||
setAndroidVersionCode: jest.fn().mockResolvedValue(''),
|
||||
setEngineExitCode: jest.fn().mockResolvedValue(''),
|
||||
setBuildVersion: vi.fn().mockResolvedValue(''),
|
||||
setAndroidVersionCode: vi.fn().mockResolvedValue(''),
|
||||
setEngineExitCode: vi.fn().mockResolvedValue(''),
|
||||
},
|
||||
}));
|
||||
|
||||
jest.mock('./model/cli/cli', () => ({
|
||||
vi.mock('./model/cli/cli', () => ({
|
||||
Cli: {
|
||||
InitCliMode: jest.fn().mockReturnValue(false),
|
||||
InitCliMode: vi.fn().mockReturnValue(false),
|
||||
},
|
||||
}));
|
||||
|
||||
jest.mock('./model/mac-builder', () => ({
|
||||
vi.mock('./model/mac-builder', () => ({
|
||||
__esModule: true,
|
||||
default: {
|
||||
run: jest.fn().mockResolvedValue(0),
|
||||
run: vi.fn().mockResolvedValue(0),
|
||||
},
|
||||
}));
|
||||
|
||||
jest.mock('./model/platform-setup', () => ({
|
||||
vi.mock('./model/platform-setup', () => ({
|
||||
__esModule: true,
|
||||
default: {
|
||||
setup: jest.fn().mockResolvedValue(''),
|
||||
setup: vi.fn().mockResolvedValue(''),
|
||||
},
|
||||
}));
|
||||
|
||||
const mockedBuildParametersCreate = BuildParameters.create as jest.Mock;
|
||||
const mockedBuildParametersCreate = BuildParameters.create as Mock;
|
||||
|
||||
function createMockBuildParameters(overrides: Record<string, any> = {}) {
|
||||
return {
|
||||
@@ -95,12 +105,12 @@ function createMockBuildParameters(overrides: Record<string, any> = {}) {
|
||||
async function runIndex(overrides: Record<string, any> = {}): Promise<void> {
|
||||
mockedBuildParametersCreate.mockResolvedValue(createMockBuildParameters(overrides));
|
||||
|
||||
return new Promise<void>((resolve) => {
|
||||
jest.isolateModules(() => {
|
||||
require('./index');
|
||||
});
|
||||
setTimeout(resolve, 100);
|
||||
});
|
||||
// index.ts exports `runMain` for testability (the file used to rely on
|
||||
// top-level execution + jest's `vi.isolateModules`, but vitest 4 dropped
|
||||
// that API). Calling the exported function directly is cleaner than
|
||||
// round-tripping through dynamic imports.
|
||||
const { runMain } = await import('./index');
|
||||
await runMain();
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -112,7 +122,7 @@ describe('index.ts plugin lifecycle wiring', () => {
|
||||
const originalEnvironment = { ...process.env };
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
vi.clearAllMocks();
|
||||
process.env.GITHUB_WORKSPACE = '/workspace';
|
||||
Object.defineProperty(process, 'platform', { value: 'linux' });
|
||||
|
||||
@@ -132,16 +142,23 @@ describe('index.ts plugin lifecycle wiring', () => {
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
describe('local build with plugin installed', () => {
|
||||
it('should call lifecycle hooks in order: initialize → beforeLocalBuild → [build] → afterLocalBuild → handlePostBuild', async () => {
|
||||
it('should call lifecycle hooks in order: initialize -> beforeLocalBuild -> [build] -> afterLocalBuild -> handlePostBuild', async () => {
|
||||
const callOrder: string[] = [];
|
||||
mockPlugin.initialize.mockImplementation(async () => callOrder.push('initialize'));
|
||||
mockPlugin.beforeLocalBuild.mockImplementation(async () => callOrder.push('beforeLocalBuild'));
|
||||
mockPlugin.beforeLocalBuild.mockImplementation(async () =>
|
||||
callOrder.push('beforeLocalBuild'),
|
||||
);
|
||||
mockPlugin.afterLocalBuild.mockImplementation(async () => callOrder.push('afterLocalBuild'));
|
||||
mockPlugin.handlePostBuild.mockImplementation(async () => callOrder.push('handlePostBuild'));
|
||||
|
||||
await runIndex();
|
||||
|
||||
expect(callOrder).toEqual(['initialize', 'beforeLocalBuild', 'afterLocalBuild', 'handlePostBuild']);
|
||||
expect(callOrder).toEqual([
|
||||
'initialize',
|
||||
'beforeLocalBuild',
|
||||
'afterLocalBuild',
|
||||
'handlePostBuild',
|
||||
]);
|
||||
});
|
||||
|
||||
it('should pass buildParameters and workspace to initialize', async () => {
|
||||
@@ -178,7 +195,6 @@ describe('index.ts plugin lifecycle wiring', () => {
|
||||
|
||||
describe('plugin handles build (canHandleBuild = true)', () => {
|
||||
it('should call handleBuild instead of Docker.run', async () => {
|
||||
const { Docker } = require('./model');
|
||||
mockPlugin.canHandleBuild.mockReturnValue(true);
|
||||
mockPlugin.handleBuild.mockResolvedValue({ exitCode: 0 });
|
||||
|
||||
@@ -206,7 +222,6 @@ describe('index.ts plugin lifecycle wiring', () => {
|
||||
|
||||
describe('fallback to local build', () => {
|
||||
it('should do a local build when handleBuild returns fallbackToLocal', async () => {
|
||||
const { Docker } = require('./model');
|
||||
mockPlugin.canHandleBuild.mockReturnValue(true);
|
||||
mockPlugin.handleBuild.mockResolvedValue({ exitCode: -1, fallbackToLocal: true });
|
||||
|
||||
@@ -225,7 +240,6 @@ describe('index.ts plugin lifecycle wiring', () => {
|
||||
|
||||
describe('no plugin installed', () => {
|
||||
it('should build locally without errors when providerStrategy is local', async () => {
|
||||
const { Docker } = require('./model');
|
||||
mockLoadPlugin.mockResolvedValue(undefined);
|
||||
|
||||
await runIndex({ providerStrategy: 'local' });
|
||||
@@ -234,12 +248,13 @@ describe('index.ts plugin lifecycle wiring', () => {
|
||||
});
|
||||
|
||||
it('should error when providerStrategy is non-local and no plugin', async () => {
|
||||
const core = require('@actions/core');
|
||||
mockLoadPlugin.mockResolvedValue(undefined);
|
||||
|
||||
await runIndex({ providerStrategy: 'aws' });
|
||||
|
||||
expect(core.setFailed).toHaveBeenCalledWith(expect.stringContaining('requires @game-ci/orchestrator'));
|
||||
expect(core.setFailed).toHaveBeenCalledWith(
|
||||
expect.stringContaining('requires @game-ci/orchestrator'),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -249,14 +264,15 @@ describe('index.ts plugin lifecycle wiring', () => {
|
||||
|
||||
describe('plugin installed but canHandleBuild returns false with non-local provider', () => {
|
||||
it('should error when providerStrategy is non-local', async () => {
|
||||
const core = require('@actions/core');
|
||||
mockPlugin.canHandleBuild.mockReturnValue(false);
|
||||
|
||||
await runIndex({ providerStrategy: 'aws' });
|
||||
|
||||
// The plugin is initialized but says it can't handle the build,
|
||||
// and providerStrategy is not local, so it falls to the error case
|
||||
expect(core.setFailed).toHaveBeenCalledWith(expect.stringContaining('requires @game-ci/orchestrator'));
|
||||
expect(core.setFailed).toHaveBeenCalledWith(
|
||||
expect.stringContaining('requires @game-ci/orchestrator'),
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user