feat: update test statuses as they are received #1075
Conversation
| @@ -191,11 +191,8 @@ export class JestTestRun implements JestExtOutput, TestRunProtocol { | |||
| return this.options.end(); | |||
| } | |||
|
|
|||
| if (this.parentRun) { | |||
| this.parentRun.end(); | |||
There was a problem hiding this comment.
This line seems to break a lot of the tests. Not sure if it is a real problem though, or it can be ignored. Also not sure how to solve it.
There was a problem hiding this comment.
Is this change just a refactoring? I noticed the refactored logic is not the same as the original. Is there any other reason for changing the logic?
There was a problem hiding this comment.
Is this change just a refactoring? I noticed the refactored logic is not the same as the original. Is there any other reason for changing the logic?
I actually added a comment on this line myself on the pull request.
It's what makes the tests fail around remembering to end the runs.
However, with this line on, when one of the tests switch status to green, the rest of them will also go out of the "running spinner" state, although they are still running.
Do you have any idea of why this happens?
There was a problem hiding this comment.
However, with this line on, when one of the tests switch status to green, the rest of them will also go out of the "running spinner" state, although they are still running.
My guess (without actually tracing the calls) is the code assumed batch mode, so once an "end" event is received, it will try to close the run. The TestExplorer will then mark all items "done" in that run accordingly. You might need to introduce a new stage/event when processing the partial result, or at least not trigger the "end" event before the whole run actually finishes...
|
@connectdotz any chance you can help out here? |
There was a problem hiding this comment.
@ffMathy Thanks for exploring this. Sorry I was late to respond.
This is not a complete review since it's still in draft mode, just answering your question and discussing some high level thoughts:
- how frequently are we processing the aggregated results? If there are 10 tests, do we end up processing the first test 10 times, 2nd test 9 times etc? Does this cause any performance issues for large projects (with lots of tests)?
- did you get a chance to test nested describe blocks with dynamic tests (it.each for example) yet? I wonder if those complex tests will fail the partial result handling until the test structure is completed anyway. I.e. we waste a lot of resources without getting any early result updates...
| @@ -191,11 +191,8 @@ export class JestTestRun implements JestExtOutput, TestRunProtocol { | |||
| return this.options.end(); | |||
| } | |||
|
|
|||
| if (this.parentRun) { | |||
| this.parentRun.end(); | |||
There was a problem hiding this comment.
Is this change just a refactoring? I noticed the refactored logic is not the same as the original. Is there any other reason for changing the logic?
No worries at all, and thanks for the preliminary review! I'll listen to your comments and make some changes soon. Probably within a couple of weeks.
I need to investigate this. Will get back to you. I think it only updates for every test file that is run, not for each individual test. I don't think this is going to mean a lot, but I could be wrong. It also depends on if we can live with breaking changes in the editor support package. If not, then we have to do mappings here, which may be costly.
No, not yet. Wanted to make sure I was in the right direction before doing an extensive test. I'll do that and get back to you! Do we have some repo with a lot of tests that can be used to try it out on? Or is it enough to use it to run the vscode-jest repo tests do you think? |
If that is the case then we should be good for the context matching. I think this is the right scope for this feature.
I am not sure if we want to do mapping either. Let's discuss this in jest-editor-support.
We don't really have a public testing repo, you can try with vscode-jest itself, as it has pretty complex test patterns. It's a good start if you can get vscode-jest run smoothly with the change... BTW, I just thought about test coverage info... this will most likely need to be processed at the end of the test run. How does the reporter handle the coverage? |
This fixes #1061.
Requires jest-community/jest-editor-support#110 to be merged first.