hbenl / vscode-test-explorer

The VS Code Test Explorer extension
MIT License
215 stars 55 forks source link

Multiple 'start' with single 'finished' #141

Closed matepek closed 4 years ago

matepek commented 4 years ago

I know it should be. an issue of the api, but since this is the implementation I though I open it here.

What is the situation? Since it is allowed to click on play button next to test name while tests are already running, I though I can support sequentially adding new tests to the batch: When run is called I just return a global promise and schedule the new tests. Works fine but on the ui it doesn't show much just when I send the running event about the new tests. If I could send multiple started events with a single finished, that would give enough flexibility.

(Now the next start assumes that there is a missing finished. I guess this is an error handling logic now, so no one should depend on this.)

Bests,

matepek commented 4 years ago

Or another event which indicates that more tests will be run

hbenl commented 4 years ago

You could already implement it like this:

Do you see any problems with this solution?

matepek commented 4 years ago

I had that solution but the problem the same: No UI feedback: When I start running a test you replace all the icons to blue/pending ones. That is what I'm missing from the second run. Either in case of your solution (which was my old solution) and in case my new solution I cannot achieve that the icon of tests/suites became blue/pending.

We can avoid the confusion by adding a new run event. With that I could send more test id's and you can change the icons: like TestRunExtendedEvent

hbenl commented 4 years ago

Damn, I hadn't thought about the blue icons for scheduled tests. Looks like a TestRunExtendedEvent may indeed be the best solution.

matepek commented 4 years ago

I'm not sure about the naming though. extend, append, ...

And since we are talking about events... I have this concept called 'static event'. It means basically that I don't have to do anything I already have the result of some tests/leafs. When a run of a suite starts at first I send those events and then I do the operation and send the rest of the events based on the result of the operation. Here is a tree.

Events are the following in order:

  1. TestRunStarted
  2. TestSuiteEvent - running Suite1
  3. TestEvent - running Static1-1
  4. TestEvent - passed/errored/.. Static1-1
  5. TestSuiteEvent - completed Suite1
  6. TestSuiteEvent - running Suite2
  7. TestEvent - running Static2-1
  8. TestEvent - passed/errored/.. Static2-1
  9. TestSuiteEvent - completed Suite2
  10. TestSuiteEvent - running Suite1
  11. TestEvent - running Dynamic1-1
  12. TestEvent - passed/errored/.. Dynamic1-1
  13. TestSuiteEvent - completed Suite1
  14. TestSuiteEvent - running Suite2
  15. TestEvent - running Dynamic2-1
  16. TestEvent - passed/errored/.. Dynamic2-1
  17. TestSuiteEvent - completed Suite2
  18. TestRunFinishedEvent

The 'problem' here is 5 and 9. Sending those event's kills the blue/pending icon for the Dynamic1-1 and Dynamic2-1. (kills means changes to back to greyed normal). Eventually the tests were running so functionally it's ok, but not nice.

I could: not sending 5,9 and 10,14 but then I need some extra logic to check that other tests will be run under that suite, etc.. totally makes sense. But.. I'm a lazy person and I've tried (only on my machine) the following:

  1. TestRunStarted
  2. ~TestSuiteEvent - running Suite1~
  3. ~TestEvent - running Static1-1~
  4. TestEvent - passed/errored/.. Static1-1
  5. ~TestSuiteEvent - completed Suite1~
  6. ~TestSuiteEvent - running Suite2~
  7. ~TestEvent - running Static2-1~
  8. TestEvent - passed/errored/.. Static2-1
  9. ~TestSuiteEvent - completed Suite2~
  10. TestSuiteEvent - running Suite1
  11. TestEvent - running Dynamic1-1
  12. TestEvent - passed/errored/.. Dynamic1-1
  13. TestSuiteEvent - completed Suite1
  14. TestSuiteEvent - running Suite2
  15. TestEvent - running Dynamic2-1
  16. TestEvent - passed/errored/.. Dynamic2-1
  17. TestSuiteEvent - completed Suite2
  18. TestRunFinishedEvent

And it seems working as I expected and good for me. Can you approve this abbreviated usage?

(If yes, do I really need the TestRunStarted and TestRunFinishedEvent events at all for only static events? I guess yes because I can imagine TestRunFinishedEvent might indicates some post processing. Don't know)

hbenl commented 4 years ago

I came up with a solution that I like more than having a new event: First of all, let's see what happens when you simply report 2 separate (though overlapping) test runs: you'd send the 2 started events for both runs as soon as the user requests them and the 2 finished events as soon as the corresponding run is finished. The result would be that each started event colors its corresponding tests blue (good) but when the first run is finished, all blue icons would disappear - including those for the second test run (bad). The problem is that when Test Explorer receives a TestRunFinishedEvent while multiple test runs are active, it doesn't know which test run is finished. I'd like to fix this by adding a testRunId property to all test run events. So when the TestRunFinishedEvent for the first run is received, only the icons for the first run are changed back to grey. I think this should fix your problem, right?

As for sending test events without the corresponding suite events: yes, that is explicitly allowed (it's even mentioned in the documentation somewhere that the test suite events are optional).

matepek commented 4 years ago

As for sending test events without the corresponding suite events: yes, that is explicitly allowed (it's even mentioned in the documentation somewhere that the test suite events are optional).

Great, Nitpicking a bit: Note: I do not send TestEvent state:running event for the static tests either. So I don't need those either, right? And how about the TestRunStarted and Finished ones if I want to send static event, do I need those?

I came up with a solution that I like more than having a new event

Then I can forget the singleton promise-like solution (returning with the same promise at the second time as at the first time. And you at your side will handle it somehow. It seems okay at first glance, but I will think a bit more about it.

hbenl commented 4 years ago

I do not send TestEvent state:running event for the static tests either. So I don't need those either, right?

Correct.

And how about the TestRunStarted and Finished ones if I want to send static event, do I need those?

Currently you don't need them (I think - I've never tested this). However in that case it would just be a coincidence, not something I intentionally wanted to support. Generally I'd recommend staying as close to the "normal" sequence of events as possible - something that works today may not be supported by Test Explorer in the future if it's something that I never really intended to support.

hbenl commented 4 years ago

The latest version of Test Explorer's API contains the testRunId property for all test run events, so if an adapter uses this property, running multiple test runs in parallel should finally work without UI glitches.

matepek commented 4 years ago

It does something but not bullet proof yet.

Issue no.1: When the first run finishes the stop button transforms to play button. (Even if I send back the same promise for both run.)

hbenl commented 4 years ago

When the first run finishes the stop button transforms to play button.

Thanks, I fixed this in 2.19.1.

matepek commented 4 years ago

Hello, Still there is an issue. I start a long test and then I start shortly after another long test works. But If I start 3 long tests in a row the third one messes it up. :S