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

FR: don't throw away results while new test is running #201

Open
gregveres opened this issue May 18, 2020 · 4 comments
Open

FR: don't throw away results while new test is running #201

gregveres opened this issue May 18, 2020 · 4 comments

Comments

@gregveres
Copy link
Contributor

Is this a bug report or a feature request?

This is a feature request that is built on top of PR #193. I have been running Majestic for a few days with the new style summary header. I like it, but I find that my workflow is the following:

  1. have a single test file running in watch mode
  2. make a change to the test or the test subject to fix a failing test.
  3. as soon as I make the change, I go look at the results of the past failing run to see the next failing test

But as soon as I hit save in step #2, the results from the previous run vanish. Now that it is really easy to see that a test is running, it would be nice if the results from the last run were still on the screen. If I was running in a terminal console, I would be able to just scroll back in the buffer to see the previous results so I could start working on the next failing test while the tests continued to run.

But right now with the tests vanishing, I have to wait for the test run to finish before I can see what the next failing test is. It would be nice if the test result didn't vanish while the test is running.

Version Info

  • Version of Majestic: v.1.7.0 - modified with PR#193
  • Version of Jest: 25
  • Version of Node:
  • Operating System:

Reproduction Repo

  1. load majestic on your test suite
  2. select a single test file
  3. click watch in the tree panel
  4. click run on the test file - make sure there is a failing test or two
  5. once the tests finish running, save your test file again

At this point you will notice that the test result (with the failures) vanishes as the summary background is now animated to indicate that the test is running. It would be nice if the test results were still shown so I could look at the next failure while the test is running.

@gregveres
Copy link
Contributor Author

gregveres commented May 18, 2020

@Raathigesh
[update: see the last message in this thread. I found a very low change way to address this]

I started looking at a solution to this, but my knowledge of react and apollo is limited. I think the real problem here is that test-file/index.tsx is a functional component rather than a class based component. My understanding of the functional component is that they have no context and have to create the output completely from the input. But in this case, the only input they are given is now the "no result" result from the "changeToResult" subscription.

I was hoping that each time there was a result, that I could save that result and use this saved version when the changeToResult passed me an "empty result". But there is no where to save the result.
I guess I could create a variable outside the function and save it there. This will work since there is only a single instance of the TestFile. If it was a class component, I would make it a class instance.

Since you understand the architecture better, I wonder if you have an idea of a better way to solve the issue.

I note that the event of starting a new test run does not have its own subscription. Instead, it seems to issue a changeToResult event with an empty result. That is what I was triggering the isUpdated on. But a better solution might be to change the events / subscriptions so that instead of the changeToResult, there was actually a testRunStarted event and a testRunFinished event. In that case, the testRunStarted wouldn't send a test result and the result would be sent in the testRunFinished event.

This would let us set isUpdating true on the testRunStarted and false on the testRunFinished.

Actually as I talk this through, I realize that I don't really understand how the functional component works with respect to these events.

Anyway, some guidance would be appreciated. With this change, I think Majestic becomes a fantastic workflow for working on a single file.

@gregveres
Copy link
Contributor Author

OK, creating a cached result that is at the file scope for test-file/index.tsx works, but it feels hacky.

I was playing around a bit more and I see that when I switch between different tests, the client asks the server for the results. So the server is caching the results per file somewhere. This leads me to think that changing the server events is the better change. And that starting a new test run shouldn't invalidate the test cache on the server until the test run result comes back.

I will see if I can learn how all that works on the server side.

@gregveres
Copy link
Contributor Author

Already I think I have found a solution.
In workspace/resolver.ts I see there are two types of event Ids that trigger this subscription: TEST_RESULT and TEST_START. So instead of creating a brand new, empty TestFileResult for the TEST_START, I think the solution is to look up this files latest test result and if we find one, then send it along in the new empty TestFileResult. This provides the UI with an empty summary but the previous test results. This allows the isUpdating from the previous PR to still catch this test start state and it provides the previous test results so they are still shown.

I think I am going to see if I can "grey out" the test results during the test run so that there is yet another visual cue that these are old test results that will change soon. I should have a PR for you to look over soon. (although I have messed up my branches in my fork). I have been trying to keep a branch that corresponds to your fork and one that has my collapsableUI and any other changes as my fork's master.

@gregveres
Copy link
Contributor Author

gregveres commented May 25, 2020

@Raathigesh
There are a couple bugs with this commit. I am noticing some cases that aren't handled. I am going to articulate them here and when I get a chance, I will see about fixing them. As I encounter others related to this change, I will add them to the list and get them fixed - hopefully by the end of the weekend:

  1. when there is an error (TS or JS syntax error), the summary banner keeps animating rather than stopping.
  2. when in watch mode and you click on the update snapshot button, the summary banner does not animate while the snapshot is running.
  3. summary banner doesn't start animating soon enough when following these steps:
    • run a test file without watch mode so that you have results (usually it will include a failing result)
    • click the watch button
    • click the run button
    • observe that the summary does not animate until the test files have been compiled and are about to be run. The summary banner should be animated starting at the time that the run button is pressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant