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

Add an option to execute a task before running/debugging tests. #192

Closed
wants to merge 1 commit into from

Conversation

bzarco
Copy link
Contributor

@bzarco bzarco commented May 10, 2020

This change introduces "test.dependsOnTask", a config setting to define the name of a user
defined task (e.g. in tasks.json) to be executed before
running/debugging tests.

This allows to perform pre-run actions (such as rebuild) before running/debugging tests, without having to introduce shell scripts (and loose debugging) or change the C++ test entry code just for the extension.

Fixes #116.

This change introduces "test.dependsOnTask", a config setting to define the name of a user
defined task (e.g. in tasks.json) to be executed before
running/debugging tests.
@matepek
Copy link
Owner

matepek commented May 10, 2020

Hello,
Wow you are so active, great :)

Let's have a quick discussion about this feature. I'm not fully against it. but slightly deeper understanding would be great how would like to use this feature.
As you can see on that issue, for rebuilding should not be necessary to have this since the extension works the other way around. It detects the change and reruns the related tests.

Also this is the second PR of yours and I'm not sure how much else is coming.. so I would like to mention that this kind of advanced options I would prefer to insert into the test.advancedExecutables instead of the global settings. That give better flexibility too.

EDITED

@bzarco
Copy link
Contributor Author

bzarco commented May 10, 2020

I am done for now :) I have been using these features for a week or so to test them, just didn't have the time to put the PRs together...

We use bazel to build, so there is no centralized command that builds tests. Instead tests are built individually by using their own target. Having to manually trigger the test build before running doesn't really work, since we may have a file open that is not the test (changing the code being tested for example), so a build task won't work (how do we know what test to build). Also, it is very easy to forget to rebuild (as the text explorer and the codelens make it very easy to run/debug!), which will run a stale version of the test and could lead to extra work (if I actually fix the issue but the test still fails... I may do more unnecessary changes). With this change bazel can be called with just the test targets being run, so it makes it super useful as we can just change code around and run/debug without any extra step to remember.

Does it make sense? I am happy to discuss further any issues you may have.

@matepek
Copy link
Owner

matepek commented May 10, 2020

Thanks for explaining. I see your use case. Let me implement it in the mentioned way.
PR's are still welcome though 😅

@matepek matepek closed this May 10, 2020
@matepek
Copy link
Owner

matepek commented May 10, 2020

Btw..

I know could be refactored here or there. (I have a full time job hehe)
But how hard was to contribute? Any feedbacks?

@bzarco bzarco deleted the depends-on-task branch May 10, 2020 03:11
@matepek
Copy link
Owner

matepek commented May 10, 2020

Would you show me how the task look's like which you are depending on?
I think I got the idea, but wanna make sure.

@matepek
Copy link
Owner

matepek commented May 10, 2020

@bzarco Is it okay if in case of running one task will be executed for each runnable?

Your solution is like collecting all the executables and passing it as ${execs} but I would rather call the task with all the executables and then one can use the ${absPath} or whatever they want.

I understand that this triggers your build system multiple times which cause some overhead but since it will be used like clicking on the test's or suite's play button it might not a big issue. Except when you run all your tests. Well..

@bzarco
Copy link
Contributor Author

bzarco commented May 10, 2020

I initially had it like that, it is unfortunately not a good solution when running multiple tests, as it becomes unmanageable very quickly. There is no way of automatically dismissing the task output once finished (if the user wants to see progress by setting reveal as always), and you get many task outputs. In our case the build system locks, so it serializes the tests and takes forever (we have hundreds of runnables...), and there is no way of knowing what task is running.

I assume most build systems will be either a single build command with incremental capabilities (in which case rebuilding makes sense as a single step before running all tests), or like ours where each test is built individually... In our case I can pass all targets to the build system at once, and it is smart to not duplicate common compilations, so it is obviously much faster (multiple calls will have incremental capabilities, but there is still duplicated work). It also works nice with reveal as always, as only a single output terminal is open and you clearly see what is going on and when tests will start running (not to mention error handling where if the single build command fails you have it in a single output to deal with it).

Here is a simplified version of the task I have now (no windows yet...), note that ${execs} being a space separated list of quoted executables allows both unix and windows to handle it (as well as having spaces in the paths)...

{
  "label": "test-explorer-pre-run",
  "type": "shell",
  "command": "TESTS=(${execs}); TEST_TARGETS=(); for TEST in \"${TESTS[@]}\"; do TEST_RELATIVE=\"${TEST##*/build-root/}\"; TEST_TARGETS+=(\"//$(dirname \"${TEST_RELATIVE}\"):$(basename \"${TEST_RELATIVE}\")\"); done; build-command \"${TEST_TARGETS[@]}\")",
  "windows": {
    "command": "for %%s in (${execs}) do echo %%s"
  },
  "presentation": {
    "reveal": "always"
  }
}

@matepek
Copy link
Owner

matepek commented May 10, 2020

I guess I need to fix some issues but let's see how is it works for you.
Version 3.0.4 is released.

@bzarco
Copy link
Contributor Author

bzarco commented May 10, 2020

Thank you Mate, it is working great! You added so much more functionality in just one day... amazing :) Also I like how your implementation changes the test icon so you know they are doing something, great work!

Just fyi, I noticed that when running multiple tests some/all get "retired" right after running, and seems they are not running in parallel anymore? (I only see one with the blue play icon at a time, the rest have a blue dash).

Thank you again, your solution solves all my issues and more!

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

Successfully merging this pull request may close these issues.

How to run command before tests?
2 participants