From ba0cff5d56dc2049364b21dda9d7881cd7ade950 Mon Sep 17 00:00:00 2001 From: Paul Cowan Date: Mon, 1 Feb 2021 17:54:26 +0000 Subject: [PATCH 1/5] ensure bundler errors go right up the stack --- .changeset/twenty-wombats-switch.md | 5 +++++ packages/server/src/manifest-builder.ts | 1 + 2 files changed, 6 insertions(+) create mode 100644 .changeset/twenty-wombats-switch.md diff --git a/.changeset/twenty-wombats-switch.md b/.changeset/twenty-wombats-switch.md new file mode 100644 index 000000000..d53fecc28 --- /dev/null +++ b/.changeset/twenty-wombats-switch.md @@ -0,0 +1,5 @@ +--- +"@bigtest/server": patch +--- + +ensure bundler errors go right up the stack diff --git a/packages/server/src/manifest-builder.ts b/packages/server/src/manifest-builder.ts index f50fb89d7..37f09ac8b 100644 --- a/packages/server/src/manifest-builder.ts +++ b/packages/server/src/manifest-builder.ts @@ -128,6 +128,7 @@ export function* createManifestBuilder(options: ManifestBuilderOptions): Operati } catch(error) { console.debug("[manifest builder] error loading manifest"); bundlerSlice.update(() => ({ type: 'ERRORED', error })); + throw error; } break; From 94fb585fc4766a48d8771a9a3a00b36508854c40 Mon Sep 17 00:00:00 2001 From: Paul Cowan Date: Tue, 2 Feb 2021 09:29:18 +0000 Subject: [PATCH 2/5] a better no default export test --- .../server/test/fixtures/exceptions/no-default-export.t.js | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 packages/server/test/fixtures/exceptions/no-default-export.t.js diff --git a/packages/server/test/fixtures/exceptions/no-default-export.t.js b/packages/server/test/fixtures/exceptions/no-default-export.t.js new file mode 100644 index 000000000..c327375e7 --- /dev/null +++ b/packages/server/test/fixtures/exceptions/no-default-export.t.js @@ -0,0 +1,3 @@ +import { test } from '@bigtest/suite'; + +export const t = test("No default export"); \ No newline at end of file From b894ed25ed9c561a7bfe641ead711c121f08e877 Mon Sep 17 00:00:00 2001 From: Paul Cowan Date: Tue, 2 Feb 2021 17:19:20 +0000 Subject: [PATCH 3/5] validate the test structure in validate-test --- packages/server/src/manifest-builder.ts | 1 - .../exceptions/invalid-test-object.t.js | 3 ++ packages/server/test/manifest-builder.test.ts | 40 ++++++++++++++++--- packages/suite/src/validate-test.ts | 17 ++++++++ 4 files changed, 54 insertions(+), 7 deletions(-) create mode 100644 packages/server/test/fixtures/exceptions/invalid-test-object.t.js diff --git a/packages/server/src/manifest-builder.ts b/packages/server/src/manifest-builder.ts index 37f09ac8b..f50fb89d7 100644 --- a/packages/server/src/manifest-builder.ts +++ b/packages/server/src/manifest-builder.ts @@ -128,7 +128,6 @@ export function* createManifestBuilder(options: ManifestBuilderOptions): Operati } catch(error) { console.debug("[manifest builder] error loading manifest"); bundlerSlice.update(() => ({ type: 'ERRORED', error })); - throw error; } break; diff --git a/packages/server/test/fixtures/exceptions/invalid-test-object.t.js b/packages/server/test/fixtures/exceptions/invalid-test-object.t.js new file mode 100644 index 000000000..01dcc41fc --- /dev/null +++ b/packages/server/test/fixtures/exceptions/invalid-test-object.t.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'no assertions or children' +} \ No newline at end of file diff --git a/packages/server/test/manifest-builder.test.ts b/packages/server/test/manifest-builder.test.ts index 4a7ee02b5..1009bc4d3 100644 --- a/packages/server/test/manifest-builder.test.ts +++ b/packages/server/test/manifest-builder.test.ts @@ -1,4 +1,4 @@ -import { describe as suite, beforeEach, it } from 'mocha'; +import { describe, beforeEach, it } from 'mocha'; import expect from 'expect'; import path from 'path'; import rmrf from 'rimraf'; @@ -22,7 +22,7 @@ const FIXTURES_DIR = path.resolve('test', 'fixtures'); const { mkdir, copyFile, readFile } = fs.promises; -const describe = process.platform === 'win32' ? suite.skip : suite; +// const describe = process.platform === 'win32' ? suite.skip : suite; describe('manifest builder', () => { let atom: Slice; let resultPath: string; @@ -145,7 +145,7 @@ describe('manifest builder', () => { }); }); - describe('importing the manifest with an error adds the error to the state', () => { + describe('importing the manifest with a syntax error adds the error to the state', () => { beforeEach(async () => { await copyFile(path.join(FIXTURES_DIR, 'exceptions', 'error.t.js'), MANIFEST_PATH); await actions.fork(atom.slice('bundler').once(({ type }) => type === 'ERRORED')); @@ -177,9 +177,37 @@ describe('manifest builder', () => { // assert is used to type narrow also and does more than just assert assertBundlerState(bundlerState.type, {is: 'ERRORED'}) - let error = bundlerState.error; + expect(bundlerState.error.message).toEqual('bork') + }); + }); - expect(error.message).toEqual('bork') + describe('importing a test with no default export adds the error to the state', () => { + beforeEach(async () => { + await copyFile(path.join(FIXTURES_DIR, 'exceptions', 'no-default-export.t.js'), MANIFEST_PATH); + await actions.fork(atom.slice('bundler').once(({ type }) => type === 'ERRORED')); }); - }) + + it('should update the global state with the error detail', () => { + let bundlerState = atom.get().bundler; + + assertBundlerState(bundlerState.type, {is: 'ERRORED'}) + + expect(bundlerState.error.message).toContain('default export') + }); + }); + + describe('importing an invalid test object adds the error to the state', () => { + beforeEach(async () => { + await copyFile(path.join(FIXTURES_DIR, 'exceptions', 'invalid-test-object.t.js'), MANIFEST_PATH); + await actions.fork(atom.slice('bundler').once(({ type }) => type === 'ERRORED')); + }); + + it('should update the global state with the error detail', () => { + let bundlerState = atom.get().bundler; + + assertBundlerState(bundlerState.type, {is: 'ERRORED'}) + + expect(bundlerState.error.message).toContain('Test contains no assertions or children') + }); + }); }); diff --git a/packages/suite/src/validate-test.ts b/packages/suite/src/validate-test.ts index 84892a648..d3524cf7f 100644 --- a/packages/suite/src/validate-test.ts +++ b/packages/suite/src/validate-test.ts @@ -19,6 +19,14 @@ export class TestValidationError extends Error { } } +type ArrayValidationFn = typeof Array.prototype.some | typeof Array.prototype.every; + +function validateTestKeys (test: Test, keys: (keyof Test)[], validationFn: ArrayValidationFn): boolean { + // the disable comment below is because eslint is not recognising k as used in !!test?.[k]. + // eslint-disable-next-line @typescript-eslint/no-unused-vars + return validationFn.call(keys, (k: keyof Test) => !!test?.[k]); +} + function findDuplicates(array: T[], callback: (value: T) => void) { let ledger = new Set(); for(let element of array) { @@ -50,6 +58,14 @@ export function validateTest(test: Test): true { throw new TestValidationError(`Invalid Test: is too deeply nested, maximum allowed depth of nesting is ${MAXIMUM_DEPTH}\n\nTest: ${path.join(' → ')}`, file) } + if ( validateTestKeys(test, ['description'], Array.prototype.every) === false) { + throw new TestValidationError(`Invalid Test: Test contains no description.\n\nDoes the test file contain a default export? Test: ${path.join(' → ')}`, file); + } + + if ( validateTestKeys(test, ['assertions', 'children'], Array.prototype.some) === false) { + throw new TestValidationError(`Invalid Test: Test contains no assertions or children.\n\nTest: ${[test.description].join(' → ')}`, test.path); + } + findDuplicates(test.assertions.map((a) => a.description), (duplicate) => { throw new TestValidationError(`Invalid Test: contains duplicate assertion: ${JSON.stringify(duplicate)}\n\nTest: ${path.join(' → ')}`, file) }); @@ -64,5 +80,6 @@ export function validateTest(test: Test): true { return true; } + return validateTestInner(test, [test.description], test.path); } From 187bd32194e19414d2250dcc98685608f732e04e Mon Sep 17 00:00:00 2001 From: Paul Cowan Date: Tue, 2 Feb 2021 17:22:35 +0000 Subject: [PATCH 4/5] create locals for array functions --- packages/suite/src/validate-test.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/suite/src/validate-test.ts b/packages/suite/src/validate-test.ts index d3524cf7f..7107d8e80 100644 --- a/packages/suite/src/validate-test.ts +++ b/packages/suite/src/validate-test.ts @@ -19,9 +19,10 @@ export class TestValidationError extends Error { } } -type ArrayValidationFn = typeof Array.prototype.some | typeof Array.prototype.every; +const every = Array.prototype.every; +const some = Array.prototype.some; -function validateTestKeys (test: Test, keys: (keyof Test)[], validationFn: ArrayValidationFn): boolean { +function validateTestKeys (test: Test, keys: (keyof Test)[], validationFn: typeof some | typeof every): boolean { // the disable comment below is because eslint is not recognising k as used in !!test?.[k]. // eslint-disable-next-line @typescript-eslint/no-unused-vars return validationFn.call(keys, (k: keyof Test) => !!test?.[k]); @@ -58,11 +59,11 @@ export function validateTest(test: Test): true { throw new TestValidationError(`Invalid Test: is too deeply nested, maximum allowed depth of nesting is ${MAXIMUM_DEPTH}\n\nTest: ${path.join(' → ')}`, file) } - if ( validateTestKeys(test, ['description'], Array.prototype.every) === false) { + if ( validateTestKeys(test, ['description'], every) === false) { throw new TestValidationError(`Invalid Test: Test contains no description.\n\nDoes the test file contain a default export? Test: ${path.join(' → ')}`, file); } - if ( validateTestKeys(test, ['assertions', 'children'], Array.prototype.some) === false) { + if ( validateTestKeys(test, ['assertions', 'children'], some) === false) { throw new TestValidationError(`Invalid Test: Test contains no assertions or children.\n\nTest: ${[test.description].join(' → ')}`, test.path); } From 6e9ec72cc6807d717e8a3252e3bb51a80119c635 Mon Sep 17 00:00:00 2001 From: Paul Cowan Date: Tue, 2 Feb 2021 17:23:05 +0000 Subject: [PATCH 5/5] use path in validate-test --- packages/suite/src/validate-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/suite/src/validate-test.ts b/packages/suite/src/validate-test.ts index 7107d8e80..7fa2faec6 100644 --- a/packages/suite/src/validate-test.ts +++ b/packages/suite/src/validate-test.ts @@ -64,7 +64,7 @@ export function validateTest(test: Test): true { } if ( validateTestKeys(test, ['assertions', 'children'], some) === false) { - throw new TestValidationError(`Invalid Test: Test contains no assertions or children.\n\nTest: ${[test.description].join(' → ')}`, test.path); + throw new TestValidationError(`Invalid Test: Test contains no assertions or children.\n\nTest: ${path.join(' → ')}`, test.path); } findDuplicates(test.assertions.map((a) => a.description), (duplicate) => {