-
Notifications
You must be signed in to change notification settings - Fork 463
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
Add parallel launch in test explorer #3577
Conversation
testName is passed only to complete the ctestArgs which is also in argument. Modifying the ctestArgs in the caller will allow concatenation of different testNames in the caller to launch ctest on a collection of tests.
Letting a '|' at the end of the regex implies selecting all tests
@microsoft-github-policy-service agree |
testName is passed only to complete the ctestArgs which is also in argument. Modifying the ctestArgs in the caller will allow concatenation of different testNames in the caller to launch ctest on a collection of tests.
Letting a '|' at the end of the regex implies selecting all tests
@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. |
There was a problem hiding this 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.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 d = driverMap.get(_driver.sourceDir); | ||
driverMap.set(_driver.sourceDir, { driver: d!.driver, ctestPath: d!.ctestPath, ctestArgs: d!.ctestArgs, tests: d!.tests.concat(test) }); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@gcampbell-msft the project i'm using to test this PR is: https://gitlab.com/themys/readers. 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 Thanks! |
@gcampbell-msft i did the tests. Second with However, with
|
Responding to your points.
|
Hi, I am very interested in this improvements. |
This will support if you select multiple tests and only run those. Is that what you're referring to? |
@gcampbell-msft i'm sorry but i can't see your responses to the remarks i did in #3577 (comment) and two others.
I'm totally ok with your proposal. I tested it and it works.
Ok.
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! |
@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. |
@hippo91 @gcampbell-msft yes that's what I meant, this is great news thanks for the confirmation. |
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). |
@@ -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; |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
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; |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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.
const d = driverMap.get(_driver.sourceDir); | ||
driverMap.set(_driver.sourceDir, { driver: d!.driver, ctestPath: d!.ctestPath, ctestArgs: d!.ctestArgs, tests: d!.tests.concat(test) }); |
There was a problem hiding this comment.
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.
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:
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? |
@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 !). |
This change addresses item #3322
This changes visible behavior and performance
The following changes are proposed:
ctest
to be run in parallel from the test explorerctest
;ctest
;cmake.ctest.AllowParallelJobs: true
)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, thentest 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!