From 8e440d5974be6c005f6e14147f0aab3ea77429fa Mon Sep 17 00:00:00 2001 From: Greg Veres Date: Sat, 8 Feb 2020 11:19:07 -0500 Subject: [PATCH 1/4] added repeatable ID generation for jest elements This is the first step to collapsable describe blocks in the UI. This change modifies the way that Ids are assigned to the jest blocks that are passed to the UI. Before this change, every time the file was refreshed, the Id assigned to a block would be brand new and different. With this change we keep track of the Ids that have been assigned and we re-use them when we recognize that the element is the same as before. This will allow us in the UI to keep track of which describe blocks have been collapsed and when we revisit the page or rerun the tests, the describe blocks that were collapsed will remain collapsed. This is just the server side of that change. --- server/services/ast/idManager.ts | 88 ++++++++++++++++++++++++++++++++ server/services/ast/inspector.ts | 84 +++++++++++------------------- 2 files changed, 119 insertions(+), 53 deletions(-) create mode 100644 server/services/ast/idManager.ts diff --git a/server/services/ast/idManager.ts b/server/services/ast/idManager.ts new file mode 100644 index 0000000..e9fe476 --- /dev/null +++ b/server/services/ast/idManager.ts @@ -0,0 +1,88 @@ +// This file manages a cache of Ids. The purpose for this cache in Majestic is to +// maintain consistent ids for test and describe blocks that do not change name between +// file reloads. By maintaining consistent Ids for the same blocks, we can do things in +// the UI like persistent open/collapse state +// +// How this works: Each describe block within a file and the file itself gets its own IdManager +// The IdManager maintains the list of Ids for that block. These could be Ids of other describe +// blocks, or they could be Ids of the tests themselves. The IdManager is also responsible for +// handing out the ids so that it can then associate the Id with the object. +// +// managing the IdManagers is done with the IdManagerFactory. It keeps a cache of the IdManagers +// and you can lookup/create an IdManager based on an Id or file path. +// +// How to use these classes: +// +// When loading in a file, call IdManagerFactory.getManagerForFile with the file's full path +// this will return an IdManger to use for the file. Set this as the current IdManager +// +// when finding a describe block, cal IdManagerFactory.getManagerForBlock with the Id of the +// describe block, this becomes the current IdManager for everythign in the describe block +// +// when finding any type of block and we need an Id for it, we call currentManager.createId +// it will return the proper Id to use for that element, either an existing one or a new one + +import * as nanoid from "nanoid"; + +interface IdEntry { + name: string; + id: string; +} + +export class IdManager { + private ids: IdEntry[] + constructor() { + this.ids = []; + } + + public createId(name: string): string { + for(let e of this.ids) { + if (e.name === name) { + return e.id; + } + } + // name was not found in the list of Ids so create it + let newId = { + name, + id: nanoid() + }; + this.ids.push(newId); + return newId.id; + } +} + +interface FileManagerEntry { + filePath: string; + idManager: IdManager; +} +export class IdManagerFactory { + static describeManagers: {key: string}; + static fileManagers: FileManagerEntry[]; + + static init() { + IdManagerFactory.describeManagers = {} as {key: string}; + IdManagerFactory.fileManagers = []; + } + + static getManagerForBlock(id: string): IdManager { + let existingManager = IdManagerFactory.describeManagers[id]; + if (existingManager === undefined) { + existingManager = new IdManager(); + IdManagerFactory.describeManagers[id] = existingManager; + } + return existingManager; + } + static getManagerForFile(filePath: string): IdManager { + for(let e of IdManagerFactory.fileManagers) { + if (e.filePath === filePath) return e.idManager; + } + let newManager = { + filePath, + idManager: new IdManager() + }; + IdManagerFactory.fileManagers.push(newManager); + return newManager.idManager; + } +} + +IdManagerFactory.init(); diff --git a/server/services/ast/inspector.ts b/server/services/ast/inspector.ts index 20c1be4..5334564 100644 --- a/server/services/ast/inspector.ts +++ b/server/services/ast/inspector.ts @@ -1,8 +1,8 @@ import traverse from "@babel/traverse"; -import * as nanoid from "nanoid"; import { parse } from "./parser"; import { readFile } from "fs"; import { TestItem, TestItemType } from "../../api/workspace/test-item"; +import {IdManagerFactory, IdManager} from './idManager'; export async function inspect(path: string): Promise { return new Promise((resolve, reject) => { @@ -24,11 +24,12 @@ export async function inspect(path: string): Promise { } const result: TestItem[] = []; + const fileIdManager = IdManagerFactory.getManagerForFile(path); traverse(ast, { CallExpression(path: any) { if (path.scope.block.type === "Program") { - findItems(path, result); + findItems(path, result, fileIdManager); } } }); @@ -46,14 +47,19 @@ function getTemplateLiteralName(path: any) { `\`\$\{${ path.node.arguments[0].expressions[currentExpressionIndex++].name }\}\`` - ); + ); } else { return finalText.concat(q.value.raw); } }, ""); } +function getNodeName(path: any): string { + return (path.node.arguments[0].type === "TemplateLiteral") + ? getTemplateLiteralName(path) + : path.node.arguments[0].value; +} -function findItems(path: any, result: TestItem[], parentId?: any) { +function findItems(path: any, result: TestItem[], idManager: IdManager, parentId?: any) { let type: string; let only: boolean = false; if (path.node.callee.name === "fdescribe") { @@ -80,66 +86,38 @@ function findItems(path: any, result: TestItem[], parentId?: any) { } if (type === "describe") { - let describe: any; - if (path.node.arguments[0].type === "TemplateLiteral") { - describe = { - id: nanoid(), - type: "describe" as TestItemType, - name: getTemplateLiteralName(path), - only, - parent: parentId - }; - } else { - describe = { - id: nanoid(), + const nodeName = getNodeName(path); + let describe = { + id: idManager.createId(nodeName), type: "describe" as TestItemType, - name: path.node.arguments[0].value, + name: nodeName, only, parent: parentId }; - } result.push(describe); path.skip(); path.traverse({ CallExpression(itPath: any) { - findItems(itPath, result, describe.id); + findItems(itPath, result, IdManagerFactory.getManagerForBlock(describe.id), describe.id); } }); } else if (type === "it") { - if (path.node.arguments[0].type === "TemplateLiteral") { - result.push({ - id: nanoid(), - type: "it", - name: getTemplateLiteralName(path), - only, - parent: parentId - }); - } else { - result.push({ - id: nanoid(), - type: "it", - name: path.node.arguments[0].value, - only, - parent: parentId - }); - } + const nodeName = getNodeName(path); + result.push({ + id: idManager.createId(nodeName), + type: "it", + name: nodeName, + only, + parent: parentId + }); } else if (type === "todo") { - if (path.node.arguments[0].type === "TemplateLiteral") { - result.push({ - id: nanoid(), - type: "todo", - name: getTemplateLiteralName(path), - only, - parent: parentId - }); - } else { - result.push({ - id: nanoid(), - type: "todo", - name: path.node.arguments[0].value, - only, - parent: parentId - }); - } + const nodeName = getNodeName(path); + result.push({ + id: idManager.createId(nodeName), + type: "todo", + name: nodeName, + only, + parent: parentId + }); } } From 89c04ac70f2ffae5db009d676ac38e0251cab753 Mon Sep 17 00:00:00 2001 From: Greg Veres Date: Sat, 8 Feb 2020 22:26:38 -0500 Subject: [PATCH 2/4] Added collapsing describe blocks This checkin adds the ability to collapse the describe blocks for the test result. The collapsed state is stored per describe block in a static class and updated each time the user toggles a block. The collapse state storage is small, only storing data for the describe blocks that are actually toggled. When a test is re-run, if there are failures in a describe block that is collapsed, it is automatically opened. I also fixed the display of the status icon beside describe blocks. Before this change it was always green. Now it is red when one of its tests fail. I am pretty sure this was a bug before because as soon as I changed the status to failed for the describe block, it changed the colour to red, indicating that the testIndicator component already handled the case but wasn't being passed the correct values. I believe this change fits in with the style of the existing component. I retained the functional component style by using React hooks. Unexpectantly, I had to use the useEffect tied to changing of the results variable. This was necessary to get the describe block to open after a test run if a test within it failed. I also had to change the implementation of allChildrenPassing. it now recursively looks at the children where it didnt before. Also, I made it so that clicking on the entire bar collapsed or expands the block rather than just the +/- div. --- ui/test-file/collapseStore.ts | 24 +++++++++++++++ ui/test-file/test-item.tsx | 56 +++++++++++++++++++++++++++-------- 2 files changed, 68 insertions(+), 12 deletions(-) create mode 100644 ui/test-file/collapseStore.ts diff --git a/ui/test-file/collapseStore.ts b/ui/test-file/collapseStore.ts new file mode 100644 index 0000000..9498891 --- /dev/null +++ b/ui/test-file/collapseStore.ts @@ -0,0 +1,24 @@ +// This static class keeps track of the collapse state of every describe block that is shown in the UI. +// It starts empty and grows with each describe block that the user collapses or expands. +// If a describe block has never been collapsed it does not have an entry in the strucure and isCollapsed will +// always return false. This allows us to get away with an empty structure to start with. +// If we ever add Redux to the product, then this can move into Redux + +export class CollapseStore { + static store: {[key: string]: boolean} + + static init() { + CollapseStore.store = {} as {[key: string]: boolean}; + } + + static isCollapsed(id: string): boolean { + return CollapseStore.store[id] === true; + } + + static setState(id: string, state: boolean) { + CollapseStore.store[id] = state; + } +} + +// Create the global instance of the store. +CollapseStore.init(); \ No newline at end of file diff --git a/ui/test-file/test-item.tsx b/ui/test-file/test-item.tsx index 1e29694..f2072f1 100644 --- a/ui/test-file/test-item.tsx +++ b/ui/test-file/test-item.tsx @@ -1,10 +1,11 @@ -import React, { Fragment } from "react"; +import React, { useState, useEffect } from "react"; import styled from "styled-components"; import { TestFileItem } from "./transformer"; import { TestFileResult } from "../../server/api/workspace/test-result/file-result"; import TestIndicator from "./test-indicator"; import { color, space } from "styled-system"; import * as Convert from "ansi-to-html"; +import { CollapseStore } from "./collapseStore"; const convert = new Convert({ colors: { @@ -21,6 +22,17 @@ function getResults(item: TestFileItem, testResult: TestFileResult) { return testResult.testResults.find(result => result.title === item.name); } +function childResultStatus (child: TestFileItem, testResult: TestFileResult): boolean { + if (child.type === "it") { + const childResult = getResults(child, testResult as any); + return (childResult == null) || (childResult.status === "passed" || childResult.status === "pending"); + } + if (child.children) { + return child.children.every(child => childResultStatus(child, testResult)); + } + return true; +} + const Container = styled.div` ${color}; ${space}; @@ -62,6 +74,13 @@ const Duration = styled.span` color: #fcd101; `; +const ViewToggle = styled.div` + font-size: 15px; + padding-right: 10px; + padding-left: 10px; + cursor: pointer; +`; + function escapeHtml(unsafe: string) { return unsafe .replace(/&/g, "&") @@ -77,7 +96,7 @@ interface Props { } export default function Test({ - item: { name, only, children }, + item: { id, name, only, children }, item, result }: Props) { @@ -85,22 +104,35 @@ export default function Test({ const isDurationAvailable = testResult && testResult.duration !== undefined; const haveFailure = testResult && testResult.failureMessages.length > 0; const allChildrenPassing = (children || []).every(child => { - if (child.type === "it") { - const childResult = getResults(child, result as any); - return childResult && childResult.status === "passed"; - } - - return true; + return childResultStatus(child, result as any); }); + + const [hideChildren, setHideChildren] = useState(CollapseStore.isCollapsed(id) && allChildrenPassing); + useEffect(() => { + // we need to use the useEffect hook because the initial value passed to state only gets set when the component is + // created the first time. The useEffect hook will allow us to force the failed describe blocks open when the + // results change. + setHideChildren(CollapseStore.isCollapsed(id) && allChildrenPassing); + }, [result]); + const toggleShowChildern = () => { + const newState = !hideChildren; + setHideChildren(newState); + CollapseStore.setState(id, newState); + } return ( - + toggleShowChildern()}> - {children && + {children && !hideChildren && children.map(child => ( ))} From 8e67b5c64db6fc379e3291c94a280140c7332ded Mon Sep 17 00:00:00 2001 From: Greg Veres Date: Sun, 9 Feb 2020 17:06:47 -0500 Subject: [PATCH 3/4] fixed issue where tests with the same name would share the same failure results Since the UI was matching results with tests by the test title, it was picking the wrong test result for all but the the first test with the duplicated name. This fix uses the previous change for collapsing describe blocks. The Ids that the backend assigned to the tests, are propogated through to the UI with the test results with this change. This allows the UI to use the id to match the test results with the tests. The end result is that a user can have the same named test in multiple locations in the test file and the UI will always match up the correct results with the tests. The id field had to be added to the test item result entry and the queries the UI sends had to include the field. Assigning Ids to the tests in the test result is fairly efficient because it can use the property of the test results that tests are grouped by under their describe block. So on each iteration of the loop, we first check to see if the existing idManaager is still valid and if it is, we just use that one rather than searching for the right idManager to use. --- server/api/workspace/resolver.ts | 1 + .../workspace/test-result/test-item-result.ts | 3 +++ server/services/results.ts | 19 +++++++++++++++++++ ui/test-file/result.gql | 1 + ui/test-file/subscription.gql | 1 + ui/test-file/test-item.tsx | 2 +- 6 files changed, 26 insertions(+), 1 deletion(-) diff --git a/server/api/workspace/resolver.ts b/server/api/workspace/resolver.ts index 76c343f..094b53e 100644 --- a/server/api/workspace/resolver.ts +++ b/server/api/workspace/resolver.ts @@ -56,6 +56,7 @@ export default class WorkspaceResolver { result.numPendingTests = payload.numPendingTests; result.testResults = payload.testResults; result.consoleLogs = payload.console; + this.results.addTestIdsToTestResults(result); this.results.setTestReport(payload.path, result); this.notifySummaryChange(); }); diff --git a/server/api/workspace/test-result/test-item-result.ts b/server/api/workspace/test-result/test-item-result.ts index 20ac4c4..3f2ee1f 100644 --- a/server/api/workspace/test-result/test-item-result.ts +++ b/server/api/workspace/test-result/test-item-result.ts @@ -19,4 +19,7 @@ export class TestItemResult { @Field() duration: number; + + @Field() + id: string; } diff --git a/server/services/results.ts b/server/services/results.ts index f092f4a..6ccdaee 100644 --- a/server/services/results.ts +++ b/server/services/results.ts @@ -5,6 +5,8 @@ import { join } from "path"; import { MajesticConfig } from "./types"; import { spawnSync } from "child_process"; import { createLogger } from "../logger"; +import { IdManager, IdManagerFactory } from "./ast/idManager"; +import { TestFileResult } from "../api/workspace/test-result/file-result"; const log = createLogger("Results"); @@ -83,6 +85,23 @@ export default class Results { } } + public addTestIdsToTestResults(results: TestFileResult) { + const fileIds = IdManagerFactory.getManagerForFile(results.path); + let currentParentName: string = ''; + let currentParentIds: IdManager = undefined as any; + for(const r of results.testResults) { + if (r.ancestorTitles[r.ancestorTitles.length-1] !== currentParentName) { + currentParentIds = fileIds; + for(const p of r.ancestorTitles) { + const parentId = currentParentIds.createId(p); + currentParentIds = IdManagerFactory.getManagerForBlock(parentId); + currentParentName = p; + } + } + r.id = currentParentIds.createId(r.title); + } + } + public getResult(path: string) { return this.results[path] || null; } diff --git a/ui/test-file/result.gql b/ui/test-file/result.gql index 76ce22b..cd967f3 100644 --- a/ui/test-file/result.gql +++ b/ui/test-file/result.gql @@ -11,6 +11,7 @@ query Results($path: String!) { failureMessages ancestorTitles duration + id } consoleLogs { message diff --git a/ui/test-file/subscription.gql b/ui/test-file/subscription.gql index 6fddfff..e574664 100644 --- a/ui/test-file/subscription.gql +++ b/ui/test-file/subscription.gql @@ -11,6 +11,7 @@ subscription Results($path: String!) { failureMessages ancestorTitles duration + id } consoleLogs { message diff --git a/ui/test-file/test-item.tsx b/ui/test-file/test-item.tsx index f2072f1..2fe2541 100644 --- a/ui/test-file/test-item.tsx +++ b/ui/test-file/test-item.tsx @@ -19,7 +19,7 @@ function getResults(item: TestFileItem, testResult: TestFileResult) { return null; } - return testResult.testResults.find(result => result.title === item.name); + return testResult.testResults.find(result => result.id === item.id); } function childResultStatus (child: TestFileItem, testResult: TestFileResult): boolean { From 9fe44e4fbbfe82ec00b8ad8f4c51e767ff59c5a9 Mon Sep 17 00:00:00 2001 From: Greg Veres Date: Thu, 16 Apr 2020 21:29:22 -0400 Subject: [PATCH 4/4] added ability to click on an error and open the offending file in VSCode This adds the ability to quickly jump to the error in the source file on a failing test. This fixes issue #186 --- server/api/app/resolver.ts | 18 ++++++++++++++++++ ui/test-file/open-failure.gql | 3 +++ ui/test-file/test-item.tsx | 13 ++++++++++++- 3 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 ui/test-file/open-failure.gql diff --git a/server/api/app/resolver.ts b/server/api/app/resolver.ts index 83df6ab..ee847d2 100644 --- a/server/api/app/resolver.ts +++ b/server/api/app/resolver.ts @@ -44,4 +44,22 @@ export default class AppResolver { return ""; } + + @Mutation(returns => String) + openFailure(@Arg("failure") failure: string) { + // The following regex matches the first line of the form: \w at () + // it captures and returns that in the second position of the match array + let re = new RegExp('^\\s+at.*?\\((.*?)\\)$', 'm'); + let match = failure.match(re); + if (match && match.length === 2) { + const path = match[1]; + launch(path, process.env.EDITOR || "code", (path: string, err: any) => { + console.log("Failed to open file in editor. You may need to install the code command to your PATH if you are using VSCode: ", err); + }); + } + else { + console.log("Failed to find a path to a file to load in the failure string."); + } + return ""; + } } diff --git a/ui/test-file/open-failure.gql b/ui/test-file/open-failure.gql new file mode 100644 index 0000000..2eb6b5b --- /dev/null +++ b/ui/test-file/open-failure.gql @@ -0,0 +1,3 @@ +mutation OpenFailure($failure: String!) { + openFailure(failure: $failure) +} diff --git a/ui/test-file/test-item.tsx b/ui/test-file/test-item.tsx index 2fe2541..c22817a 100644 --- a/ui/test-file/test-item.tsx +++ b/ui/test-file/test-item.tsx @@ -6,6 +6,8 @@ import TestIndicator from "./test-indicator"; import { color, space } from "styled-system"; import * as Convert from "ansi-to-html"; import { CollapseStore } from "./collapseStore"; +import OPEN_FAILURE from "./open-failure.gql"; +import { useMutation } from "react-apollo-hooks"; const convert = new Convert({ colors: { @@ -119,10 +121,19 @@ export default function Test({ const newState = !hideChildren; setHideChildren(newState); CollapseStore.setState(id, newState); + if (children && children.length > 0) { + } } + + const openFailure = useMutation(OPEN_FAILURE, { + variables: { + failure: testResult && testResult.failureMessages ? testResult.failureMessages[0] : '' + } + }); + return ( - toggleShowChildern()}> + (children && children.length > 0) ? toggleShowChildern() : openFailure()}>