Add parallel launch in test explorer

[hippo91]

This change addresses item #3322

This changes visible behavior and performance

The following changes are proposed:

• Allow ctest to be run in parallel from the test explorer
• The parallel run is authorized only if:
  • all tests have the same driver;
  • all tests have the same path to ctest;
  • all tests have the same arguments given to ctest;
  • (by the way i cannot figure out when those previous conditions maybe false)
  • all tests have the same customized task;
  • all tests have the same consumer
  • the configuration allows parallel run (cmake.ctest.AllowParallelJobs: true)
• If the parallel run is authorized, a big regex holding all the test names is built and then passed to the runCTestImpl method. I suppose that performance issue may appear if the regex is really big. I tested it on one of my project with a hundred tests and it seems to be ok.

Other Notes/Information

It appears that test result analysis is based on the analyze of the Test.xml file generated by CTest. However CTest does generate it only if the option -T Test is given. This option is set by default in the config but if the user removes it, then test result won't occur. Maybe we should to add this option in the ctest args manually.

The implementation proposed here is very naive and i am very new to Typescript programming but it seems to be functional. Maybe it is a good starting point to solve #3322.

I tested manually this MR on one of my projects and it gives me satisfaction but i don't know how to add tests to check this MR in the CI. I don't even know if it is relevant.

Last thing, thank you for the hard work on this so useful VSCode extension!

[hippo91]

@microsoft-github-policy-service agree

[hippo91]

Hi @gcampbell-msft.

Sorry to bother you but i do not have any news about this MR. I fully understand that you (and other maintainers) should have a lot of work to do. I just wanted to know if i forgot to do something ?

Thanks a lot for the hard work. Best regards

[gcampbell-msft]

@hippo91 Thank you for the ping and the PR! We are currently in the process of having recently released and likely doing a patch release with some bug fixes in the next week or so. Due to our release process, we won't be taking / reviewing PR's until after that patch release.

However, I'll make sure we do our best to review this after that process is finished.

[hippo91]

Ok nice! Thanks @gcampbell-msft !

[hippo91]

@gcampbell-msft sorry to ping you. I just want to be sure this MR has not been forgotten.

[gcampbell-msft]

@hippo91 No worries. I have it on my backlog to look at soon. Our goal is to address this before releasing 1.18. Thanks!

[hippo91]

Ok :+1: thanks a lot @gcampbell-msft !

[gcampbell-msft]

@hippo91 Could you help me to understand the changes of this PR by giving a general description of how the code works after your changes? I have a base level understanding by reading through it, but it'd be helpful to hear you explain it. Thanks

[hippo91]

@gcampbell-msft the basic idea behind this MR is to call ctest only once for all tests instead of looping over them and calling ctest for each one.

We achieve this by collecting the arguments of each runCTestImplcalls into sets. The idea is that, if the arguments are identical for all calls (i.e only one item in each set) then we realize only one call for all of those tests. This is done by building a regex that aggregate all of the tests name before passing it to runCTestImpl function.

In other words instead of having a loop over the tests and launch ctest -R name_of_the_test, we are calling ctest only once : ctest -R regex_for_all_tests.

It is very naive but i think it can respond to many use cases.

I hope i have been clear

[gcampbell-msft]

@gcampbell-msft the basic idea behind this MR is to call ctest only once for all tests instead of looping over them and calling ctest for each one.

We achieve this by collecting the arguments of each runCTestImplcalls into sets. The idea is that, if the arguments are identical for all calls (i.e only one item in each set) then we realize only one call for all of those tests. This is done by building a regex that aggregate all of the tests name before passing it to runCTestImpl function.

In other words instead of having a loop over the tests and launch ctest -R name_of_the_test, we are calling ctest only once : ctest -R regex_for_all_tests.

It is very naive but i think it can respond to many use cases.

I hope i have been clear

Thank you for the context! I added a commit that slightly cleans up the code, but I think these changes make sense. I'm now working to make sure I fully understand the context, as I think with the realization of these changes and possibly some other things I have in mind, we can improve the testing experience in general. I think it's safe to say that we plan to take this PR, but I'm in the midst of possibly adding some more minor changes to the PR. Thank you so much for the contribution!

[gcampbell-msft]

@hippo91 I'm curious, in order to assist me with testing some of the things I'm trying, would you be able to share your project? You mentioned it having 100's of tests, it'd be great to test on.

[gcampbell-msft]

@hippo91 I've made a couple of changes that keep the integrity of your change, but with what I believe are some slight improvements. Please review and let me know if you're happy with these changes!

@xisui-MSFT I'd love your thoughts.

Also, pinging @elizabethmorrow @moyo1997 @qarni for review.

[hippo91]

@hippo91 I'm curious, in order to assist me with testing some of the things I'm trying, would you be able to share your project? You mentioned it having 100's of tests, it'd be great to test on.

@gcampbell-msft the project i'm using to test this PR is: https://gitlab.com/themys/readers. You will need to work inside a docker container to have all the required dependencies. The image to use is here: https://gitlab.com/themys/dockerimagefactory/container_registry/6252392

If you need more help i'll be happy to assist you.

[gcampbell-msft]

@hippo91 I'm curious, in order to assist me with testing some of the things I'm trying, would you be able to share your project? You mentioned it having 100's of tests, it'd be great to test on.

@gcampbell-msft the project i'm using to test this PR is: https://gitlab.com/themys/readers. You will need to work inside a docker container to have all the required dependencies. The image to use is here: https://gitlab.com/themys/dockerimagefactory/container_registry/6252392

If you need more help i'll be happy to assist you.

Actually, it would be of great help if you could test your project using the most recent code! Specifically both with the allowParallelJobs setting enabled and disabled.

Thanks!

[hippo91]

@gcampbell-msft i did the tests. First with allowParallelJobs disabled, each test is run separately as expected (see image below).

Second with allowParallelJobs enabled, all tests are run simultaneously as expected (see image below). Only one ctest command is launched for all tests.

However, with allowParallelJobs enabled, three things are to notice:

1. the tests are not running in parallel due to the fact that the ctest command does not have -j 20 as arguments (see my previous remark https://github.com/microsoft/vscode-cmake-tools/pull/3577#discussion_r1566955606).

2. the tests status are not updated until the ctest command returns but it is conform to the message in the description of the allowParallelJobs option: The test explorer may not accurately reflect test progress

3. when clicking on the square button to interrupt the tests, vscode update the tests status but the ctest command is still running until its end (which is conform to the code). See image below:

[gcampbell-msft]

@gcampbell-msft i did the tests. First with allowParallelJobs disabled, each test is run separately as expected (see image below).

Second with allowParallelJobs enabled, all tests are run simultaneously as expected (see image below). Only one ctest command is launched for all tests.

However, with allowParallelJobs enabled, three things are to notice:

1. the tests are not running in parallel due to the fact that the ctest command does not have -j 20 as arguments (see my previous remark Add parallel launch in test explorer #3577 (comment)).
2. the tests status are not updated until the ctest command returns but it is conform to the message in the description of the allowParallelJobs option: The test explorer may not accurately reflect test progress
3. when clicking on the square button to interrupt the tests, vscode update the tests status but the ctest command is still running until its end (which is conform to the code). See image below:

Responding to your points.

1. This goes back to my response here: https://github.com/microsoft/vscode-cmake-tools/pull/3577#discussion_r1566955606. I'm open to other thoughts, but I think that the setting is whether or not we allow the parallel jobs. I was thinking it makes more sense to rely on our getCTestArgs method. However, I'd be okay with defaulting to include it, but allowing user settings to override if it's not included. I can make that change.
2. Correct, this is expected and was the case when I pulled your changes as well, we won't have a way to update them in real time because of it only being one invocation.
3. Interesting, was this the case with your original changes? I didn't change anything here, and it might be that we need additional logic to do this. I notice that even before our changes, this was an issue, so I think this might be better handled in a separate PR.

[triou-harmonicinc]

Hi, I am very interested in this improvements. I would like to know if it's gonna support running only a subset of the tests in parallel based on the current test filter or if that needs to be tackled in a separate issue ?

[gcampbell-msft]

Hi, I am very interested in this improvements. I would like to know if it's gonna support running only a subset of the tests in parallel based on the current test filter or if that needs to be tackled in a separate issue ?

This will support if you select multiple tests and only run those. Is that what you're referring to?

[hippo91]

@gcampbell-msft i'm sorry but i can't see your responses to the remarks i did in https://github.com/microsoft/vscode-cmake-tools/pull/3577#discussion_r1566928288 and two others. But it doesn't really matter as:

This goes back to my response here: https://github.com/microsoft/vscode-cmake-tools/pull/3577#discussion_r1566955606. I'm open to other thoughts, but I think that the setting is whether or not we allow the parallel jobs. I was thinking it makes more sense to rely on our getCTestArgs method. However, I'd be okay with defaulting to include it, but allowing user settings to override if it's not included. I can make that change.

I'm totally ok with your proposal. I tested it and it works.

Correct, this is expected and was the case when I pulled your changes as well, we won't have a way to update them in real time because of it only being one invocation.

Ok.

Interesting, was this the case with your original changes? I didn't change anything here, and it might be that we need additional logic to do this. I notice that even before our changes, this was an issue, so I think this might be better handled in a separate PR.

I 'am almost sure it was the case in my original changes and i totally agree with you that it should be handled in a separate PR.

Thanks a lot for all your work on this PR!

[hippo91]

@triou-harmonicinc, as @gcampbell-msft said, if you select multiple tests and run only those, they will run in parallel. I tested it on my project and it worked.

[triou-harmonicinc]

@hippo91 @gcampbell-msft yes that's what I meant, this is great news thanks for the confirmation.

[triou-harmonicinc]

Correct, this is expected and was the case when I pulled your changes as well, we won't have a way to update them in real time because of it only being one invocation.

I suppose this could be tackled by live parsing ctest stdout instead on relying on the ctest output xml file until we reach the end of the ctest process. This might be a bit more error-prone though (and might have side-effects I am not aware of). Unless ctest has a live publising result feature that I don't know of.

[dgazzoni]

First of all thank you for these improvements.

I believe my VS Code and CMake Tools extension were already updated to support it (correct me if I'm wrong). I have then enabled the "CTest: Allow Parallel Jobs" option. When I try to run it, I get this error:

[ctest] RegularExpression::compile(): Expression too big.

This on code that I'm working on that should be made public very, very soon (probably by the time someone reads this message), at https://github.com/dgazzoni/PQC-AMX -- however note that to run these tests, a Mac with Apple Silicon is required. There are a little bit over 1,000 tests on this project, and some of them have long names and weird characters due to the use of Google Test's parameterized tests feature.

Can anyone confirm they also get this issue?

[hippo91]

@dgazzoni yes this issue is known, sorry for this. Please see #3804

The problem was that we built a huge regex to launch all the test in parallel. In fact it is not necessary if the user wants to launch all the tests. @gcampbell-msft did a PR to fix this behavior (by the way thanks a lot for this @gcampbell-msft !). The problem is still present if the user select a huge subset (but not all) of the whole tests. This needs to be addressed in a future PR.

[gcampbell-msft]

@hippo91 Thanks for responding!

@dgazzoni The above comment captures all of the context. The pre-release channel has these fixes, and we plan to do an official release in the near future.