Collapsable ui

[gregveres]

There are two parts to the change. The first is changing the server to reuse Id values when it rescans the test file. Before this change the Ids would all be different each rescan. If that behaviour wasn't changed, then each time the rescan happened, the collapsed blocks would spring open. So the first step was to get the back end to reuse Ids.

The second part is in the UI where I augmented the test-file component to use react hooks to add some state: the collapsed state. I ended up improving the pass/fail status of the describe blocks by recursing the children to create the state. This allowed me to spring open a collapsed block that has a test failure after a test run. Also, now describe blocks that have test failures are coloured red instead of staying green. This allows us to see which blocks contain the failed tests.
When the user switches back and forth between files, the collapsed state is remembered.

I had assumed this would fix the issue #181 but it does not. I will look at that tomorrow.

The changes in this branch fix #180.

[gregveres]

@Raathigesh can you run the integration test on this branch before merging. One of the three tests were failing on my machine right from the get go. I don't think this would have caused a failure. If I knew cypress, I would create a few integration tests for this work.

• a test that makes sure that collapsing and expanding works.
• a test for rolling up the error status of the describe blocks.
• a test to ensure that it remembers collapsed state when switching between files.
• a test that ensured that a failing test sprung open after the test finished running.

[gregveres]

Hmm, I guess I don't really understand how PRs work on github. I would have thought that the PR was a snapshot in time. But it appears that my third checkin (the one that fixes issue #181) is now included in the PR rather than allowing me to create a new PR. Sorry about that. The fix for #181 builds on the code of #180 anyway. The Ids that were created for 180 are needed for the fix for #181

[Raathigesh]

Thank you for doing this change. Appreciate it. I left a few comments to get clarity on a few of the choices made here.

[Raathigesh]

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?

[gregveres]

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.

[Raathigesh]

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.

[gregveres]

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.

[Raathigesh]

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?

[gregveres]

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.

[Raathigesh]

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.

[Raathigesh]

This is a neat refactor 👍

[gregveres]

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.

[Raathigesh]

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

[gregveres]

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.

[Raathigesh]

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

[gregveres]

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.

[gregveres]

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.

[Raathigesh]

I'm ok to keep this implementation for now.