Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collapsable ui #183

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions server/api/app/resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 <some text> (<file path>)
// it captures <file path> 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 "";
}
}
1 change: 1 addition & 0 deletions server/api/workspace/resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
3 changes: 3 additions & 0 deletions server/api/workspace/test-result/test-item-result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,7 @@ export class TestItemResult {

@Field()
duration: number;

@Field()
id: string;
}
88 changes: 88 additions & 0 deletions server/services/ast/idManager.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
// This file manages a cache of Ids. The purpose for this cache in Majestic is to
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could still build consistent IDs by combining the file path and the nested test hierarchy names so I'm trying to understand what's the benefit we get by having a manager like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the answer to the 3rd question first. It explains why we can't just store the collapsed state in the components if you care about one of the features I was trying to achieve. Then come back and read this answer.

If we assume that we need to search for the collapsed state by Id, we really don't want our Ids to be huge, because we would be doing some large string compares.

The path will be typically pretty big. I estimate that the average path would be around 500 - 1000 characters, depending on how big your titles are for describe and test blocks and how much you nest describe blocks.

The path to my typical test file is well over 100 characters and I use a lot of nesting of describe blocks and my test titles are tend to be 50-150 characters. Those are taken from my current project that has 6600+ unit tests in TS running in Jasmine. I have another 9000+ unit tests in C#.

On the server, we could quickly generate the Id as the full path from the file down to the test. This would be quick on the server, but I think it would get slow in the browser.

This structure is quite efficient as we build up small groups of strings and these strings can be much smaller because they are already scoped by the describe block (or file path) that they belong to. When running on my system, I didn't notice any delay in generating the file summary.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to generate this for all the files and all the tests in those files though.

The collapsed state is going to be stored only for the items which we collapsed so it's not going to be that big. And I don't think storing this state in memory is going to be a problem here. My worry is that we are introducing unwanted complexity with less/no benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you do have to create the full path for every test in every file and send all that data to the front end. When I was looking at fixing the bug #181, I realized that the front end doesn't have the complete path information, so we can't reconstruct the full path in the UI.

this means that if we use the full path as the ID, then it has to be generated in the server component and sent to the UI. The UI has to store all this extra information (1/2 KB per test for every test) and then when doing the display, it still as to look up in some mapping table that maps a 500 byte string to a boolen, regardless of if the string key exists or not.

Or am I missing something?

And to fix #181, in the getResults function on the line:

return testResult.testResults.find(result => result.title == item.name);

Right now this is comparing the name of the test against all the results for that file, bailing when it find the right one. This has to change to be

return testResult.testResults.find(result => result.id == item.id);

With the implementation under review, these are all fairly small Ids that are going to be randomly different. But if the Ids are the full path, this line is going to be doing an O(n^2) operation on 500 character strings that are all the same for the first 400 characters. It may seem reasonable for small test cases, but it seems like a bad choice for large projects.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The full path of a file is available here.

We could generate/calculate an ID for each test block by combining the file path and the describe/it block hierarchy.

When you first load the majestic UI, we don't need to store any collapsed state. Whenever we collapse a hierarchy only we want to keep that in the state. So when a file is rendered, we can look up all the collapsed items and not render its children.

The UI has to store all this extra information (1/2 KB per test for every test)

Regarding the above statement, We don't need to store anything unless you clicked on a hierarchy and collapse it. If there is no collapsed info in the state, that means it's expanded.

What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That isn't the full path we are talking about. That is the full path to the file. That is just the prefix to the test path, which is the nested describe blocks followed by the test name.
I must have mistated something if you thought I couldn't figure out where the file path was. That is the one thing that is all over the place. :) And the file summary isn't where we need the file path.

We need a unique Id here. In the current code base, this line is wrong. It will match the first test of that name in the file rather than the correct test result. At that point, I don't have access to the hierarchy to do a match. Based on these changes, I can change that line to test result.Id against item.id, once I added the Id to the result that is. That change is here.

You are correct that when majestic loads it doesn't need to store any collapsed state. And that is what happens with these changes. If we only keep the collapsed setting in the state of the component, then do the following steps ...

  1. view file 1
  2. collapse a describe block
  3. view file 2
  4. come back to file 1

The collapsed block will be expanded again because the state of the component will have been wiped out because it is a new component. That is why I store the state in a global indexed by the test Id. And why would use the context api instead of the global state.

To expand on the comment you asked about ... we would be storing the Id (a much bigger string if it is the full test path) in each test-item and in each result item. And when we did comparisons to change the state or to do the compare here, we would be comparing a fairly long string that is unique for the first 80% of the string.

But at this point, I really dont have the energy to argue anymore. The next time I get some time, I will modify the code to not store the Ids on the server and instead use the full test path as the Id of the test.

I wonder if we can shorten the Id by not putting the file path in there as the prefix. How likely will it be to have the exact same test path in multiple files. Pretty small I would imagine. And even if we did, the only bug would be potentially showing a block collapsed when we shouldn't. That would shave 100 bytes off the Id right there.

BTW, I have another change queued up where clicking on a test failure takes you right to the line of the source file where Jest is telling us the failed exception is. I find it a really handy addition. I have been running with it for the past week.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry! My intention is not to argue but to find a simple enough solution so it's easy to maintain the project in the long term.

I'll have a look again and get back to you on this after looking at the code in detail so you don't feel like we are going in circles. Thanks for all your effort. I really appreciate the time anyone puts into open source projects.

// 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();
84 changes: 31 additions & 53 deletions server/services/ast/inspector.ts
Original file line number Diff line number Diff line change
@@ -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<TestItem[]> {
return new Promise((resolve, reject) => {
Expand All @@ -24,11 +24,12 @@ export async function inspect(path: string): Promise<TestItem[]> {
}

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);
}
}
});
Expand All @@ -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 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a neat refactor 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I don't like repeating code - especially in JS/TS where it doesn't get compiled away. And once I realized that it could be collapsed like that, I felt it was easier to understand what is happening.

[Warning: off topic :) ]
One thing I have learned over the years is that it is much easier to understand code when it can fit on one screen. So I break things into smaller functions with describtive names. So instead of having a huge function, I will break things up like this:

function () {
  for(var element of array() ) {
    doStep1OfAlgorthm(...); // maybe this a 100 line function
    doStep2OfAlgorithm(...); // maybe another 100 line function
   if(success) {
      doCommitOfAlgorithm(...);
    }
  }
}

Since I can see the entire loop on the screen at once, I can figure out what the function is doing, once it gets too big to fit on the screen, it becomes much harder to work with.
Sorry for the divergence, I just wanted to share a bit of what I learned the hard way over the years. It has made my code much cleaner and easier to understand. And it was partially what drove this refactor. I can just get findItems on one screen now. I might take the refactoring further and collapse the setup portion into a function called:

parseTestItemAttributes(...)
that returns an object of type:string and only: boolean.

Then I might collapse the action portion into a function called:
addTestItemToResultList(...)

that way the findItems function becomes:

function findItems(path: any, result: TestItem[], idManager: IdManager, parentId?: any) {
  var pathAttributes = parseTestItemAttributes(path);
  addTestItemToResultList(path, pathAttributes);
}

but I didn't want to introduce too much change into a code base that isn't mine.

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") {
Expand All @@ -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
});
}
}
19 changes: 19 additions & 0 deletions server/services/results.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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;
}
Expand Down
24 changes: 24 additions & 0 deletions ui/test-file/collapseStore.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// This static class keeps track of the collapse state of every describe block that is shown in the UI.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use component state or context to share state than having a single store like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The components get destroyed and recreated each time you switch to a different test file. One of the aspects of this feature that I was trying to go for was for it to remember which blocks were collapsed as I switched from one test file to another and back again. For instance, I might be working in one test file and have collapsed a portion of it, then I need to check out a different test file for some information and when I come back to the first test file, I would like it to have remembered the collapse state.
My understanding of the component structure is that there is only one test-file component that keeps getting reused each time the current file is switched. This means that the test-items (where I would store the collapsed state) is getting deleted and re-created each time we swap files.

The collapsed state is global application state and if we were using Redux, then it would go there, but it didn't seem reasonable to introduce the overhead and complexity of Redux just for this. Not to mention that I am new to React and have no experience with Redux. I have a bit more experience at this point with Vue/Vuex.

So the bottom line, to answer your question is that it is a store like this because I wanted the app to remember the collapsed state as I switch between files of the project.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. We could use the Context API to share global state rather than introducing a singleton?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. I didn't know about the context API until I ran into it last week when I learned about Vue's equivalent api.
I will look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to take much longer than I have time for right now. The Context API seems much more complex when coupled with functional components like you are using. It is pretty straight forward for a class based component.
I will have to learn how to conver to a class based component or learn how to deal with the context in the functional component. At first glance it looks like using functional components means we would introduce another functional component whose job is soley to call the existing component, injecting the context. Seems clunky.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok to keep this implementation for now.

// 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();
3 changes: 3 additions & 0 deletions ui/test-file/open-failure.gql
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
mutation OpenFailure($failure: String!) {
openFailure(failure: $failure)
}
1 change: 1 addition & 0 deletions ui/test-file/result.gql
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ query Results($path: String!) {
failureMessages
ancestorTitles
duration
id
}
consoleLogs {
message
Expand Down
1 change: 1 addition & 0 deletions ui/test-file/subscription.gql
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ subscription Results($path: String!) {
failureMessages
ancestorTitles
duration
id
}
consoleLogs {
message
Expand Down