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 parallel launch in test explorer #3577

Merged

Conversation

hippo91
Copy link
Contributor

@hippo91 hippo91 commented Feb 8, 2024

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
Copy link
Contributor Author

hippo91 commented Feb 8, 2024

@microsoft-github-policy-service agree

@gcampbell-msft
Copy link
Collaborator

@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.

Copy link
Contributor Author

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcampbell-msft thanks a lot for your improvements.

It is clearly better thanks to them. I just have some interrogations but it is probably due to my lack of knowledge of some features.

Comment on lines +292 to +296
const tests = this.testItemCollectionToArray(testExplorer.items);
const run = testExplorer.createTestRun(new vscode.TestRunRequest());
const ctestArgs = await this.getCTestArgs(driver, customizedTask, testPreset);
const returnCode = await this.runCTestHelper(tests, run, driver, undefined, ctestArgs, undefined, customizedTask, consumer);
return returnCode;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcampbell-msft i do not understand why this is modified. Should we assume here that ctestAllowParallelJobs is always true?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not assumed. The runCTestHelper based on your changes as well as mine now check internally the ctestAllowParallelJobs variable, so there is no need to check it here. We can instead still be integrated with the test explorer no matter that setting.

Comment on lines +428 to +429
const d = driverMap.get(_driver.sourceDir);
driverMap.set(_driver.sourceDir, { driver: d!.driver, ctestPath: d!.ctestPath, ctestArgs: d!.ctestArgs, tests: d!.tests.concat(test) });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we assume here that all tests of the same _driver.sourceDir automatically share the same driver, ctestPath, and ctestArgs?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the assumption I'm making.

I'm treating _driver.sourceDir as a key for the driver. Our code shouldn't have more than one driver per source code, so this is a safe assumption.

Additionally, our construction of ctestPath or ctestArgs happens by using utilizing the driver, see the construction when ctestPath and ctestArgs aren't passed in by parameter. Additionally, the case where the ctestArgs is passed in from the parameter is covered as well, since these will be the same for all tests since we default to the parameter.

const uniqueCtestPath: string = driver.ctestPath;
const uniqueCtestArgs: string[] = driver.ctestArgs;

uniqueCtestArgs.push('-R');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it legit to remove the -j arg? Are we going to use all the required procs?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell, the -j arg already gets included prior to calling this method. The getCtestArgs already includes it in the case where a Test Preset is not used.

I'm open to still adding it here, but the way I thought of it was to treat the ctestAllowParallelJobs as us allowing parallel invocation of tests to happen, we still want users to have control over whether they include the -j flag. I'm open to discussion or different opinions on this though.

@hippo91
Copy link
Contributor Author

hippo91 commented Apr 16, 2024

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

hippo91 commented Apr 17, 2024

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

disabled_running

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

enabled_running

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:

enabled_stopped

@gcampbell-msft
Copy link
Collaborator

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

disabled_running

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

enabled_running

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:

enabled_stopped

Responding to your points.

  1. This goes back to my response here: Add parallel launch in test explorer #3577 (comment). 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
Copy link

triou-harmonicinc commented Apr 17, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

hippo91 commented Apr 18, 2024

@gcampbell-msft i'm sorry but i can't see your responses to the remarks i did in #3577 (comment) and two others.
But it doesn't really matter as:

This goes back to my response here: #3577 (comment). 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
Copy link
Contributor Author

hippo91 commented Apr 18, 2024

@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
Copy link

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

@triou-harmonicinc
Copy link

triou-harmonicinc commented Apr 18, 2024

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.

@@ -428,82 +430,133 @@ export class CTestDriver implements vscode.Disposable {
if (await this.runCTestHelper(children, run, _driver, _ctestPath, _ctestArgs, cancellation, customizedTask, consumer)) {
returnCode = -1;
}
return returnCode;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we start returning here? From my reading of the code, this is a recursive method so I would think we'd want the return value to come from the end of the method.

src/ctest.ts Outdated
}
}

if (!this.ws.config.ctestAllowParallelJobs || driverSet.size > 1 || ctestPathSet.size > 1 || ctestArgsSet.size > 1 || customizedTaskSet.size > 1 || consumerSet.size > 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part confuses me since in the runCTest method we already check this boolean of ctestAllowParallelJobs. What entry point are you testing this work such that it is working for you?

I believe the main reason we hadn't done this before is because the results file doesn't allow us to get a result per test,and the test explorer entry point is different.

@xisui-MSFT for any additional context.

@hippo91 Fill me in on any context I'm missing for what entry points you are using, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the test result file now support parallel tests?

src/ctest.ts Outdated
driverSet.add(_driver);
ctestPathSet.add(_ctestPath);
ctestArgsSet.add(_ctestArgs);
customizedTaskSet.add(customizedTask);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to test a set here for the customizedTask or the consumer, because these are parameters to the function, so it'll only be whatever is passed in. I think we can simply use these directly and not from the set.

Comment on lines +292 to +296
const tests = this.testItemCollectionToArray(testExplorer.items);
const run = testExplorer.createTestRun(new vscode.TestRunRequest());
const ctestArgs = await this.getCTestArgs(driver, customizedTask, testPreset);
const returnCode = await this.runCTestHelper(tests, run, driver, undefined, ctestArgs, undefined, customizedTask, consumer);
return returnCode;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not assumed. The runCTestHelper based on your changes as well as mine now check internally the ctestAllowParallelJobs variable, so there is no need to check it here. We can instead still be integrated with the test explorer no matter that setting.

const uniqueCtestPath: string = driver.ctestPath;
const uniqueCtestArgs: string[] = driver.ctestArgs;

uniqueCtestArgs.push('-R');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell, the -j arg already gets included prior to calling this method. The getCtestArgs already includes it in the case where a Test Preset is not used.

I'm open to still adding it here, but the way I thought of it was to treat the ctestAllowParallelJobs as us allowing parallel invocation of tests to happen, we still want users to have control over whether they include the -j flag. I'm open to discussion or different opinions on this though.

Comment on lines +428 to +429
const d = driverMap.get(_driver.sourceDir);
driverMap.set(_driver.sourceDir, { driver: d!.driver, ctestPath: d!.ctestPath, ctestArgs: d!.ctestArgs, tests: d!.tests.concat(test) });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the assumption I'm making.

I'm treating _driver.sourceDir as a key for the driver. Our code shouldn't have more than one driver per source code, so this is a safe assumption.

Additionally, our construction of ctestPath or ctestArgs happens by using utilizing the driver, see the construction when ctestPath and ctestArgs aren't passed in by parameter. Additionally, the case where the ctestArgs is passed in from the parameter is covered as well, since these will be the same for all tests since we default to the parameter.

@dgazzoni
Copy link

dgazzoni commented Jun 8, 2024

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
Copy link
Contributor Author

hippo91 commented Jun 8, 2024

@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
Copy link
Collaborator

@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.

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.

4 participants