From a72874794522069e694ccc82c98f6680480ca772 Mon Sep 17 00:00:00 2001 From: David Siegel Date: Thu, 15 Feb 2018 17:41:55 -0800 Subject: [PATCH 1/4] Play with Buildkite --- test/.prettierrc | 1 + test/buildkite.ts | 50 +++++++++++++++++++++++++++++++++++++++++++++++ test/fixtures.ts | 10 +++++----- test/languages.ts | 28 +++++++++++++++++--------- test/test.ts | 19 +++++++++--------- 5 files changed, 84 insertions(+), 24 deletions(-) create mode 100644 test/.prettierrc create mode 100644 test/buildkite.ts diff --git a/test/.prettierrc b/test/.prettierrc new file mode 100644 index 00000000..4415cc8d --- /dev/null +++ b/test/.prettierrc @@ -0,0 +1 @@ +{ "printWidth": 110 } diff --git a/test/buildkite.ts b/test/buildkite.ts new file mode 100644 index 00000000..5ecb1418 --- /dev/null +++ b/test/buildkite.ts @@ -0,0 +1,50 @@ +import * as _ from "lodash"; +import { exec } from "shelljs"; + +import { WorkItem } from "./test"; +import { allFixtures, Fixture } from "./fixtures"; + +function getChangedFiles(base: string, commit: string): string[] { + let diff = exec(`git fetch -v origin ${base} && git diff --name-only origin/${base}..${commit}`).stdout; + return diff.trim().split("\n"); +} + +export function affectedFixtures(changedFiles: string[] | undefined = undefined): Fixture[] { + if (changedFiles === undefined) { + const { BUILDKITE_PULL_REQUEST_BASE_BRANCH: base, BUILDKITE_COMMIT: commit } = process.env; + return commit === undefined ? allFixtures : affectedFixtures(getChangedFiles(base || "master", commit)); + } + + // We can ignore changes in Markdown files + changedFiles = _.reject(changedFiles, file => _.endsWith(file, ".md")); + + // All fixtures are dirty if any changed file is not included as a sourceFile of some fixture. + const fileDependencies = _.flatMap(allFixtures, f => f.language.sourceFiles || []); + const allFixturesDirty = _.some(changedFiles, f => !_.includes(fileDependencies, f)); + + if (allFixturesDirty) return allFixtures; + + const dirtyFixtures = allFixtures.filter( + fixture => + // Fixtures that don't specify dependencies are always dirty + fixture.language.sourceFiles === undefined || + // Fixtures that have a changed file are dirty + _.some(changedFiles, f => _.includes(fixture.language.sourceFiles, f)) + ); + + return dirtyFixtures; +} + +export function divideParallelJobs(workItems: WorkItem[]): WorkItem[] { + const { BUILDKITE_PARALLEL_JOB: pjob, BUILDKITE_PARALLEL_JOB_COUNT: pcount } = process.env; + + if (pjob === undefined || pcount === undefined) return workItems; + + try { + const segment = Math.ceil(workItems.length / parseFloat(pcount)); + const start = parseInt(pjob, 10) * segment; + return workItems.slice(start, start + segment); + } catch { + return workItems; + } +} diff --git a/test/fixtures.ts b/test/fixtures.ts index 03cd0a4d..1194b5fd 100644 --- a/test/fixtures.ts +++ b/test/fixtures.ts @@ -69,6 +69,8 @@ function jsonTestFiles(base: string): string[] { export abstract class Fixture { abstract name: string; + constructor(public language: languages.Language) {} + runForName(name: string): boolean { return this.name === name; } @@ -113,11 +115,8 @@ export abstract class Fixture { } abstract class LanguageFixture extends Fixture { - protected language: languages.Language; - constructor(language: languages.Language) { - super(); - this.language = language; + super(language); } async setup() { @@ -342,7 +341,8 @@ class JSONSchemaJSONFixture extends JSONFixture { skipMiscJSON: false, skipSchema: [], rendererOptions: {}, - quickTestRendererOptions: [] + quickTestRendererOptions: [], + sourceFiles: language.sourceFiles }; super(schemaLanguage); this.runLanguage = language; diff --git a/test/languages.ts b/test/languages.ts index 44a84146..fe5d6ef1 100644 --- a/test/languages.ts +++ b/test/languages.ts @@ -17,6 +17,7 @@ export interface Language { skipSchema: string[]; rendererOptions: RendererOptions; quickTestRendererOptions: RendererOptions[]; + sourceFiles?: string[]; } export const CSharpLanguage: Language = { @@ -41,7 +42,8 @@ export const CSharpLanguage: Language = { { "array-type": "list" }, { "csharp-version": "5" }, { density: "dense" } - ] + ], + sourceFiles: ["src/Language/CSharp.ts"] }; export const JavaLanguage: Language = { @@ -64,7 +66,8 @@ export const JavaLanguage: Language = { skipMiscJSON: false, skipSchema: ["keyword-unions.schema"], // generates classes with names that are case-insensitively equal rendererOptions: {}, - quickTestRendererOptions: [] + quickTestRendererOptions: [], + sourceFiles: ["src/Language/Java.ts"] }; export const RustLanguage: Language = { @@ -83,7 +86,8 @@ export const RustLanguage: Language = { skipSchema: [], skipMiscJSON: true, rendererOptions: {}, - quickTestRendererOptions: [{ density: "dense" }] + quickTestRendererOptions: [{ density: "dense" }], + sourceFiles: ["src/Language/Rust.ts"] }; export const GoLanguage: Language = { @@ -105,7 +109,8 @@ export const GoLanguage: Language = { skipMiscJSON: false, skipSchema: [], rendererOptions: {}, - quickTestRendererOptions: [] + quickTestRendererOptions: [], + sourceFiles: ["src/Language/Golang.ts"] }; export const CPlusPlusLanguage: Language = { @@ -130,7 +135,8 @@ export const CPlusPlusLanguage: Language = { skipMiscJSON: false, skipSchema: [], rendererOptions: {}, - quickTestRendererOptions: [{ unions: "indirection" }] + quickTestRendererOptions: [{ unions: "indirection" }], + sourceFiles: ["src/Language/CPlusPlus.ts"] }; export const ElmLanguage: Language = { @@ -164,7 +170,8 @@ export const ElmLanguage: Language = { "keyword-unions.schema" // can't handle "hasOwnProperty" for some reason ], rendererOptions: {}, - quickTestRendererOptions: [{ "array-type": "list" }] + quickTestRendererOptions: [{ "array-type": "list" }], + sourceFiles: ["src/Language/Elm.ts"] }; export const SwiftLanguage: Language = { @@ -193,7 +200,8 @@ export const SwiftLanguage: Language = { { "struct-or-class": "class" }, { density: "dense" }, { density: "normal" } - ] + ], + sourceFiles: ["src/Language/Swift.ts"] }; export const ObjectiveCLanguage: Language = { @@ -223,7 +231,8 @@ export const ObjectiveCLanguage: Language = { skipMiscJSON: false, skipSchema: [], rendererOptions: { functions: "true" }, - quickTestRendererOptions: [] + quickTestRendererOptions: [], + sourceFiles: ["src/Language/Objective-C.ts"] }; export const TypeScriptLanguage: Language = { @@ -243,5 +252,6 @@ export const TypeScriptLanguage: Language = { skipMiscJSON: false, skipSchema: ["keyword-unions.schema"], // can't handle "constructor" property rendererOptions: { "explicit-unions": "yes" }, - quickTestRendererOptions: [] + quickTestRendererOptions: [], + sourceFiles: ["src/Language/TypeScript.ts"] }; diff --git a/test/test.ts b/test/test.ts index 5fddf356..64106175 100755 --- a/test/test.ts +++ b/test/test.ts @@ -6,29 +6,28 @@ import * as _ from "lodash"; import { inParallel } from "./lib/multicore"; import { exec, execAsync, Sample } from "./utils"; import { Fixture, allFixtures } from "./fixtures"; +import { affectedFixtures, divideParallelJobs } from "./buildkite"; const exit = require("exit"); - -////////////////////////////////////// -// Constants -///////////////////////////////////// - const CPUs = parseInt(process.env.CPUs || "0", 10) || os.cpus().length; ////////////////////////////////////// // Test driver ///////////////////////////////////// -type WorkItem = { sample: Sample; fixtureName: string }; +export type WorkItem = { sample: Sample; fixtureName: string }; async function main(sources: string[]) { let fixtures = allFixtures; const fixturesFromCmdline = process.env.FIXTURE; if (fixturesFromCmdline) { const fixtureNames = fixturesFromCmdline.split(","); - fixtures = _.filter(fixtures, fixture => - _.some(fixtureNames, name => fixture.runForName(name)) - ); + fixtures = _.filter(fixtures, fixture => _.some(fixtureNames, name => fixture.runForName(name))); + } else { + fixtures = affectedFixtures(); + if (allFixtures.length !== fixtures.length) { + console.error(`* Running a subset of fixtures: ${fixtures.map(f => f.name).join(", ")}`); + } } // Get an array of all { sample, fixtureName } objects we'll run. // We can't just put the fixture in there because these WorkItems @@ -44,7 +43,7 @@ async function main(sources: string[]) { _.map(x.samples.others, s => ({ fixtureName: x.fixtureName, sample: s })) ); - const tests = _.concat(_.shuffle(priority), _.shuffle(others)); + const tests = divideParallelJobs(_.concat(priority, others)); await inParallel({ queue: tests, From 331649de6dd056da8aed562d97e1e1c4b0279034 Mon Sep 17 00:00:00 2001 From: David Siegel Date: Thu, 15 Feb 2018 21:14:22 -0800 Subject: [PATCH 2/4] Add timing info to test run messages --- test/fixtures.ts | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/test/fixtures.ts b/test/fixtures.ts index 1194b5fd..55e4c49e 100644 --- a/test/fixtures.ts +++ b/test/fixtures.ts @@ -93,24 +93,30 @@ export abstract class Fixture { return `test/runs/${this.name}-${randomBytes(3).toString("hex")}`; } - printRunMessage( + runMessageStart( sample: Sample, index: number, total: number, cwd: string, shouldSkip: boolean - ): void { + ): string { const rendererOptions = _.map( sample.additionalRendererOptions, (v, k) => `${k}: ${v}` ).join(", "); - console.error( + const message = [ `*`, chalk.dim(`[${index + 1}/${total}]`), chalk.magenta(this.name) + chalk.dim(`(${rendererOptions})`), path.join(cwd, chalk.cyan(path.basename(sample.path))), shouldSkip ? chalk.red("SKIP") : "" - ); + ].join(" "); + console.time(message); + return message; +} + + runMessageEnd(message: string) { + console.timeEnd(message); } } @@ -153,7 +159,7 @@ abstract class LanguageFixture extends Fixture { let shouldSkip = this.shouldSkipTest(sample); const additionalFiles = this.additionalFiles(sample); - this.printRunMessage(sample, index, total, cwd, shouldSkip); + const message = this.runMessageStart(sample, index, total, cwd, shouldSkip); if (shouldSkip) { return; @@ -180,6 +186,8 @@ abstract class LanguageFixture extends Fixture { }); shell.rm("-rf", cwd); + + this.runMessageEnd(message); } } From 48ef25ba93b64f1bb999bce7e6ebd968cbdce610 Mon Sep 17 00:00:00 2001 From: David Siegel Date: Thu, 15 Feb 2018 22:30:56 -0800 Subject: [PATCH 3/4] Remove priority-only test runs --- test/fixtures.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/fixtures.ts b/test/fixtures.ts index 55e4c49e..e6f43c85 100644 --- a/test/fixtures.ts +++ b/test/fixtures.ts @@ -303,10 +303,7 @@ class JSONFixture extends LanguageFixture { priority = quickTestSamples.concat(priority); } - if (IS_CI && !IS_PR && !IS_BLESSED) { - // Run only priority sources on low-priority CI branches - others = []; - } else if (IS_CI) { + if (IS_CI) { // On CI, we run a maximum number of test samples. First we test // the priority samples to fail faster, then we continue testing // until testMax with random sources. From f56bff93d918f63c2af9fd979c5bb7b202972041 Mon Sep 17 00:00:00 2001 From: David Siegel Date: Thu, 15 Feb 2018 22:31:05 -0800 Subject: [PATCH 4/4] Remove test shuffling --- test/fixtures.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/fixtures.ts b/test/fixtures.ts index e6f43c85..8c0c43c6 100644 --- a/test/fixtures.ts +++ b/test/fixtures.ts @@ -309,7 +309,6 @@ class JSONFixture extends LanguageFixture { // until testMax with random sources. const testMax = 100; others = _.chain(samplesFromPaths(miscSamples)) - .shuffle() .take(testMax - prioritySamples.length) .value(); }