hbenl / vscode-test-explorer

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

Skipped tests are executed when they retired #139

Open matepek opened 4 years ago

matepek commented 4 years ago

A user of mine noticed this.

  1. Set up autorun for root.
  2. Send retire event for all the tests and include a skipped one.

When the run event called the id of the skipped test is contained by the array.

I have to send the event because the executable has changed but the user don't want to actually run it.

hbenl commented 4 years ago

This is actually a design decision: I want to leave as much logic to the adapters as possible. So you should filter out the skipped tests in your adapter if your test framework doesn't do that for you.

matepek commented 4 years ago

I understand that but please consider this.

This way of working seems essential, otherwise how could the user force-run a skipped test. That's why I think the reasonable resolution is not sending back the skipped test ids.

I could workaround it of course like remembering that a retired event was sent about a skipped test and force ignore it but this sounds dirty. What do you think?

hbenl commented 4 years ago

OK, I understand your use case better now. Initially I had thought about filtering out skipped tests from all test run requests, but that would have made it impossible to force-run a skipped test. But I don't think we can assume this logic will be the same for all adapters, so currently I can think of 2 possible solutions:

  1. you don't send skipped tests in your retire event
  2. I add a second parameter to TestAdapter#run() that tells the adapter the reason for the request. The Test Explorer would send manual as the reason if the user clicked a run button and auto if the request is the result of the autorun feature. Your adapter could then filter out skipped tests only if the reason is auto.

What do you think?

matepek commented 4 years ago
  1. you don't send skipped tests in your retire event

If the test was run previously that it has a state which should be retired, so not retiring doesn't seem right.

  1. I add a second parameter to TestAdapter#run() ....

We could do this but what if the user selects the skipped test(s) for autorun? Then 'auto' autorun event should mean to run the test(s). Therefore filtering out would be wrong.

Would you help me to understand why we cannot assume the logic I explained to you? I don't see the problem. ^^

hbenl commented 4 years ago

If the test was run previously that it has a state which should be retired, so not retiring doesn't seem right.

If a test is skipped then its state is skipped, Test Explorer has no other state information to revert to when a skipped test is retired, so it will simply stay in its skipped state. So, while I understand that it feels wrong to exclude skipped tests from a retire event, it wouldn't actually make any difference right now (except that the skipped tests would then also not appear in the test run request, which is what you wanted).

Would you help me to understand why we cannot assume the logic I explained to you? I don't see the problem.

It's mostly a gut feeling that this kind of "hidden" logic may cause problems. One scenario I can think of where this could be problematic: in the future I want to introduce "virtual suites", i.e. the user can re-arrange the tests according to some logic. Now when the user requests to run one of these virtual suites, Test Explorer can't send a test suite id to the adapter anymore (because the adapter doesn't know anything about these virtual suites), it'll have to send the test ids - so the assumption that a request by the user to run a suite would lead to a suite id being sent to the adapter would not be valid anymore.

matepek commented 4 years ago

If a test is skipped then its state is skipped, Test Explorer has no other state information to revert to when a skipped test is retired

I got the idea that I'm unintentionally using something as a feature.. Let's go through how skipped tests are working in my case.

Whenever a run is called with an id (not plural because "autorun" feature sends just one id, but the logic can be rephrased):

So if the id is a skipped test's id then it will be run and it will have state (like other not skipped test's states). (I think it is a pretty neat feature because in case of C++ you don't have to change the source and recompile your executable to run a skipped test.)

When the exec recompiled of course the contained skipped tests should be retired too along with the not skipped ones, right?

But should them to be 'rerun': I don't think so, they are "skipped". (Unless a skipped test is enabled for autorun in which case of course but that is already handled by my original logic.)

I add a second parameter to TestAdapter#run() ....

I would be able to filter out the skipped tests but I would lose the "autorun a skipped test" feature. If you insist this is the least worst solution for my users.


Thought bit unrelated to the topic follows..

I want to introduce "virtual suites"

I've just did something like that. I have a logic which allows customization. See here and here. It's "alpha" but seems working. I think what you will need to make it more valuable is TesInfo|TestSuiteInfo having tags one day.

I was thinking about labeling/tagging tests. And this goes back to my other questions about the events

For example having a test called "test1" with "tag1" and "tag2". I could build a tree like the following:

Whenever a user clicks the play of the first test1 I would update the second test1 too. If you will do that virtual grouping in a general way that would obsolete my idea and maybe my grouping feature too. I wouldn't mind that at all.

hbenl commented 4 years ago

OK, I realize I hadn't fully thought this through: I was thinking a skipped test shows the skipped icon, so what state would it get retired to? But you're talking about the situation after the test was force-run and hence shows a passed/failed icon. Of course, that should be retired, so forget about my first solution.

But since you mentioned that the user should also be able to force-run a test by enabling autorun for just that test (and then having the executable recompiled), I don't understand anymore why you'd need Test Explorer to handle this logic: if I understand you correctly, skipped tests should be run if and only if a test run for just that test (not a suite containing the test) is requested, and it doesn't matter if that request came from autorun or was made manually. That could be handled by the adapter itself, just filter out skipped tests if a test run for more than one test is requested. What am I missing here?

About virtual suites: initially I wanted this for 2 features requested by users of my adapter:

But once such a facility is there, having tags and allowing grouping by tags is the obvious next step. The tree you described should be supported then.

matepek commented 4 years ago

I was thinking a skipped test shows the skipped icon, so what state would it get retired to?

Yes, Skipped tests can be run by clicking the play button next to them. In this case Text Explorer sends the id of it and I interpret it as "force-run". In case only the parent id is present I skip it. (code) It is really useful... (I think you cannot do such kind of thing in case of mocha.) Ergo user can run skipped tests => skipped tests can have states because I'm sending "TestEvent" for them. Retire event should retire them.

if I understand you correctly, skipped tests should be run if and only if a test run for just that test (not a suite containing the test) is requested

Close but not exactly. (code) If the id of the skipped test is explicitly contained by the test id array I get from the explorer. Why? Because multi-selection works. One can select tests one-by-one and clicking on the play button next to one of it will result that I get all the selected tests' ids.

But since you mentioned that the user should also be able to force-run a test by enabling autorun for just that test (and then having the executable recompiled), I don't understand

Let me know if the previous clarification haven't cleared it.

About virtual suites

It seems we are on the same page then. One note.. One test can have multiple tags therefore it is not a "tree" anymore. I think you aware of this, just wanted to mention.