Skip to content

Commit

Permalink
fix(sdk): more resources are created than needed for tests (#6948)
Browse files Browse the repository at this point in the history
Fixes #6316

This PR addresses an issue where if you just one to run an individual test (in the CLI or Wing Console), it simulates more resources than are needed, which can have side effects like excess Docker containers being spun up on the developer's machine.

The root cause of the issue is that to ensure test isolation, the Wing source file(s) are compiled into a Wing simulator file with multiple copies of the users app (one for each test defined in the entire app). But this assumes all of the tests are going to be executed. If only one or a handful of tests are to be executed, we don't need all of those environments.

As a solution, this PR changes the testing mechanism so a compilation for the simulator always produces a single copy of the app, and when tests are executed, each test is run sequentially, stopping and restarting the simulator as needed.

## Checklist

- [ ] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [ ] Description explains motivation and solution
- [ ] Tests added (always)
- [ ] Docs updated (only required for features)
- [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
  • Loading branch information
Chriscbr authored Jul 30, 2024
1 parent 2b18e76 commit f53a25c
Show file tree
Hide file tree
Showing 272 changed files with 720 additions and 739 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,6 @@ import { simulator } from "@winglang/sdk";

import type { Simulator } from "../../wingsdk.js";
import type { Compiler } from "../compiler.js";
import type { ConstructTreeNode } from "../construct-tree.js";

const getTestPaths = (node: ConstructTreeNode) => {
const tests: string[] = [];
const children = Object.values(node.children ?? {});

if (
node.constructInfo?.fqn === "@winglang/sdk.std.Test" &&
children.some((child) => child.id === "Handler")
) {
tests.push(node.path);
}

for (const child of children) {
tests.push(...getTestPaths(child));
}

return tests;
};

/**
* Create a simulator manager that can be used to run tests.
Expand Down Expand Up @@ -60,9 +41,7 @@ export const createSimulatorManager = ({
const getTests = async () => {
const simulator = await createSimulator();

const { tree } = simulator.tree().rawData();

return getTestPaths(tree);
return simulator.tree().listTests();
};

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import { formatTraceError } from "../format-wing-error.js";

import { createSimulatorManager } from "./simulator-manager.js";

const TEST_BATCH_SIZE = 4;

export type TestStatus = "success" | "error" | "idle" | "running";

export interface TestItem {
Expand Down Expand Up @@ -223,26 +225,31 @@ export const createTestRunner = ({
onTestsChange();

const startTime = Date.now();
const result = await simulatorManager.useSimulatorInstance(
async (simulator: Simulator) => {
const promises = testList.map(async (test) => {
const response = await executeTest(simulator, test.id, logger);

testsState.setTest({
...test,
status: response.error ? "error" : "success",
time: response.time,
datetime: Date.now(),
});
onTestsChange();

return response;
});
return await Promise.all(promises);
},
);

const testPassed = result.filter((r) => r.pass);
// Run all tests in batches.
const results = new Array<InternalTestResult>();
for (const testBatch of chunk(testList, TEST_BATCH_SIZE)) {
await Promise.allSettled(
testBatch.map(async (test) => {
await simulatorManager.useSimulatorInstance(
async (simulator: Simulator) => {
const result = await executeTest(simulator, test.id, logger);
results.push(result);

testsState.setTest({
...test,
status: result.error ? "error" : "success",
time: result.time,
datetime: Date.now(),
});
onTestsChange();
},
);
}),
);
}

const testPassed = results.filter((r) => r.pass);
const time = Date.now() - startTime;

const { default: prettyMs } = await import("pretty-ms");
Expand All @@ -269,3 +276,11 @@ export const createTestRunner = ({
},
};
};

function chunk<T>(array: Array<T>, groupSize: number): Array<Array<T>> {
const groups = [];
for (let index = 0; index < array.length; index += groupSize) {
groups.push(array.slice(index, index + groupSize));
}
return groups;
}
16 changes: 8 additions & 8 deletions apps/wing/src/commands/test/test.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -533,25 +533,25 @@ test "put" {

const BUCKET_TEST_RESULT = [
{
path: "root/env0/test:put",
path: "root/Default/test:put",
pass: true,
traces: [
{
data: { message: "Put (key=test1.txt).", status: "success" },
type: "resource",
sourcePath: "root/env0/Bucket",
sourcePath: "root/Default/Bucket",
sourceType: "@winglang/sdk.cloud.Bucket",
},
{
data: { message: "Get (key=test1.txt).", status: "success", result: '"Foo"' },
type: "resource",
sourcePath: "root/env0/Bucket",
sourcePath: "root/Default/Bucket",
sourceType: "@winglang/sdk.cloud.Bucket",
},
{
data: { message: "Invoke (payload=undefined).", status: "success" },
type: "resource",
sourcePath: "root/env0/test:put/Handler",
sourcePath: "root/Default/test:put/Handler",
sourceType: "@winglang/sdk.cloud.Function",
},
],
Expand All @@ -562,7 +562,7 @@ const OUTPUT_FILE = {
results: {
"test.test.w": {
put: {
path: "root/env0/test:put",
path: "root/Default/test:put",
pass: true,
traces: [
{
Expand All @@ -571,7 +571,7 @@ const OUTPUT_FILE = {
status: "success",
},
type: "resource",
sourcePath: "root/env0/Bucket",
sourcePath: "root/Default/Bucket",
sourceType: "@winglang/sdk.cloud.Bucket",
},
{
Expand All @@ -581,7 +581,7 @@ const OUTPUT_FILE = {
result: '"Foo"',
},
type: "resource",
sourcePath: "root/env0/Bucket",
sourcePath: "root/Default/Bucket",
sourceType: "@winglang/sdk.cloud.Bucket",
},
{
Expand All @@ -590,7 +590,7 @@ const OUTPUT_FILE = {
status: "success",
},
type: "resource",
sourcePath: "root/env0/test:put/Handler",
sourcePath: "root/Default/test:put/Handler",
sourceType: "@winglang/sdk.cloud.Function",
},
],
Expand Down
87 changes: 21 additions & 66 deletions apps/wing/src/commands/test/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,21 +449,10 @@ async function testSimulator(synthDir: string, options: TestOptions) {
let outputStream: SpinnerStream | undefined;
let traceProcessor: TraceProcessor | undefined;

let currentTestName: string | undefined;
if (options.stream) {
// As of this comment, each Wing test is associated with an isolated environment.
// (All resources for test #0 are in root/env0/..., etc.)
// This means we can use the environment number to map each environment # to a test name,
// so when we receive a trace from the simulator, we can infer which test it's associated with.
const testMappings = extractTestMappings(s.listResources());

const printEvent = async (event: std.Trace) => {
const env = extractTestEnvFromPath(event.sourcePath);

let testName = "(no test)";
if (env !== undefined) {
testName = testMappings[env] ?? testName;
}

const testName = currentTestName ?? "(no test)";
if (testFilter && !testName.includes(testFilter) && testName !== "(no test)") {
// This test does not match the filter, so skip it.
return;
Expand Down Expand Up @@ -499,21 +488,30 @@ async function testSimulator(synthDir: string, options: TestOptions) {
});
}

const results = [];

try {
await s.start();
const tests = s.tree().listTests();
const filteredTests = filterTests(tests, testFilter);
for (const t of filteredTests) {
// Set the currentTestName so all logs emitted during this test run are
// associated with the current test.
currentTestName = extractTestNameFromPath(t);

await s.start();
const testRunner = s.getResource(
`${options.rootId}/cloud.TestRunner`
) as std.ITestRunnerClient;
const result = await testRunner.runTest(t);
results.push(result);
await s.stop();
await s.resetState();
}
} catch (e) {
outputStream?.stopSpinner();
throw e;
}

const testRunner = s.getResource(`${options.rootId}/cloud.TestRunner`) as std.ITestRunnerClient;
const tests = await testRunner.listTests();
const filteredTests = filterTests(tests, testFilter);

const results = await runTests(testRunner, filteredTests);

await s.stop();

if (options.stream) {
await traceProcessor!.finish();
outputStream!.stopSpinner();
Expand Down Expand Up @@ -747,20 +745,8 @@ function sortTests(a: std.TestResult, b: std.TestResult) {
return a.path.localeCompare(b.path);
}

/**
* Take a path like "root/env123/foo/bar" and return the environment number (123).
*/
function extractTestEnvFromPath(path: string): number | undefined {
const parts = path.split("/");
const envPart = parts[1] ?? parts[0];
if (!envPart.startsWith("env")) {
return undefined;
}
return parseInt(envPart.substring(3));
}

/*
* Take a path like "root/env123/foo/test:first test/bar" and return "first test".
* Take a path like "root/foo/bar/test:first test/baz" and return "first test".
*/
function extractTestNameFromPath(path: string): string | undefined {
const parts = path.split("/");
Expand All @@ -772,37 +758,6 @@ function extractTestNameFromPath(path: string): string | undefined {
return undefined;
}

/*
* Take a list of paths like:
*
* root/env0/foo
* root/env0/test:first test <-- this is a test
* root/env1/bar/test:second test <-- this is a test
* root/env1/bar
*
* and extract the mapping from environment indices to test names:
*
* { 0: "first test", 1: "second test" }
*/
function extractTestMappings(paths: string[]): Record<number, string> {
const mappings: Record<number, string> = {};
for (const path of paths) {
const parts = path.split("/");
if (parts.some((p) => p.startsWith("test:"))) {
const env = extractTestEnvFromPath(path);
if (env === undefined) {
continue;
}
const testName = extractTestNameFromPath(path);
if (testName === undefined) {
continue;
}
mappings[env] = testName;
}
}
return mappings;
}

const MAX_BUFFER = 10 * 1024 * 1024;

/**
Expand Down
6 changes: 5 additions & 1 deletion examples/tests/invalid/unresolved_state.test.w
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// this test will fail because no body is setting "my_unresolved_token" during deployment.
// this test will fail because nobody is setting "my_unresolved_token" during deployment.

bring sim;
bring cloud;
Expand All @@ -14,3 +14,7 @@ let fn = new cloud.Function(
FOO_VALUE: state.token("my_unresolved_token")
}
);

test "" {
fn.invoke();
}
2 changes: 1 addition & 1 deletion examples/tests/valid/hello.test.w
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ let queue = new cloud.Queue();

queue.setConsumer(inflight (message: str) => {
bucket.put("wing.txt", "Hello, {message}");
});
});
2 changes: 1 addition & 1 deletion libs/awscdk/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export class App extends core.App {
this.isTestEnvironment = props.isTestEnvironment ?? false;
registerTokenResolver(new CdkTokens());

TestRunner._createTree(this, props.rootConstruct);
TestRunner._createTree(this, props.rootConstruct, this.isTestEnvironment);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion libs/awscdk/src/test-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export class TestRunner extends std.TestRunner {
);
}

constructor(scope: Construct, id: string, props: std.TestRunnerProps = {}) {
constructor(scope: Construct, id: string, props: std.TestRunnerProps) {
super(scope, id, props);

// This output is created so the CLI's `wing test` command can obtain a list
Expand Down
9 changes: 8 additions & 1 deletion libs/wingsdk/src/simulator/simulator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ export class Simulator {
await this.stop();

if (resetState) {
await rm(this.statedir, { recursive: true });
await this.resetState();
this._traces = [];
}

Expand All @@ -523,6 +523,13 @@ export class Simulator {
await this.start();
}

/**
* Reset the state of the simulator.
*/
public async resetState(): Promise<void> {
await rm(this.statedir, { recursive: true });
}

/**
* Get a list of all resource paths.
*/
Expand Down
26 changes: 26 additions & 0 deletions libs/wingsdk/src/simulator/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ export class Tree {
return structuredClone(this.data);
}

/**
* Returns a list of all tests in the tree.
* @returns a list of all tests in the tree
*/
public listTests(): string[] {
return getTestPaths(this.data.tree);
}

/**
* Returns the raw data for a specific construct node.
*/
Expand All @@ -31,3 +39,21 @@ export class Tree {
return curr;
}
}

const getTestPaths = (node: ConstructTreeNode) => {
const tests: string[] = [];
const children = Object.values(node.children ?? {});

if (
node.constructInfo?.fqn === "@winglang/sdk.std.Test" &&
children.some((child) => child.id === "Handler")
) {
tests.push(node.path);
}

for (const child of children) {
tests.push(...getTestPaths(child));
}

return tests;
};
Loading

0 comments on commit f53a25c

Please sign in to comment.