-
Notifications
You must be signed in to change notification settings - Fork 8
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 support for cpputestTestAdapter.preLaunchTask
setting
#61
Conversation
This can happen if a previous TestLoadFinishedEvent contained an errorMessage instead of a suite. No code takes advantage of this yet. It is being added in preparation for preLaunchTask support, which will emit such an event if the task fails.
The configured task will be run at the beginning of load(), run(), and debug(). This can be used to rebuild the test executable. Addresses issue bneumann#42
Anyway we can fix this race condition? It should only happen when all tests configure in parallel right? So maybe there is something in Cmake that let's us check if we need the configuration to run beforehand? |
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.
Nice! That's a good feature
await this.root.RunAllTests(); | ||
} else { | ||
await this.root.RunTest(...tests); | ||
if (await this.updateTests()) |
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.
Nicely done. If you want you could make it more "promisey" e.g. in the updateTests() throw an error to propagate it up and in the calling methods do something like:
this.updateTests
.then(success => {
...
})
.catch(err => this.testsEmitter...)
But I am fine with it as it is
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.
Any reason to use .then().catch()
instead of ordinary try/catch blocks? All the callers of updateTests()
are already async methods, doesn't that result in the same behavior under the hood? I am not as fluent in TypeScript's async/Promises as I am in, e.g., Python's, but I thought those were equivalent ways of saying the same thing.
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.
Technically it is almost the same. It is more a style thing, but I was just perusing the rest of the code and I remember that I wanted to make it more functional but didn't go through all of it anyway. So it's fine as it is. I will merge it and maybe refactor "later" :)
I did some digging about this for a while and didn't find anything that looked immediately promising. I'll say a bit more about how it's all working, maybe you will see something I'm missing or know some tricks that I don't. This feature doesn't know anything about CMake specifically. It is designed to work with any build system by using tasks. These can either be tasks provided by an extension, e.g. When the CMake Tools extension is running a configure at project open time because of So the issue is that when a new folder is opening, CMake Tools You could make the case that the "right" place to fix this is in the CMake Tools extension, by making configure and build commands wait for previous commands to complete rather than throwing an error. As I was researching to make sure I had all my ducks in a row for this explanation, I did come across another potential workaround from the one I mentioned in a comment on #42. I don't know if it will work in practice, but if it does the tradeoffs feel better to me than the ones I'm making now.
Downsides to this approach are:
The longer I look at this, the more I don't see a way out aside from patching the CMake Tools extension. Do you see something I'm overlooking? |
I have a couple of additional small quality-of-life changes I may make to this PR. It's mergable as-is but you may want to hold off for a day or two while I tweak it if you want to avoid PR churn. |
Wow, that was a very concise explanation of the problem. Thanks for that and no, I don't see a way either except maybe checking running process on the OS level to detect cmake doing something. But then again you can't be sure it's not your own process that is running. |
The configured task will be run at the beginning of
load()
,run()
, anddebug()
. This can be used to rebuild the test executable.Addresses issue #42
One minor issue I ran into is that when first opening a CMake project with
"cmake.configureOnOpen": true
, there's a race that leads to an error popup, "Configuration is already in progress". The only way to solve this that I have found is to setcmake.configureOnOpen
tofalse
, and to make the CMake Build task intasks.json
depend on the CMake Configure task. It works, but it creates a little bit of latency in various operations and requires creating custom tasks instead of being able to rely on the default templates from CMake Tools.If the task process returns a nonzero error code, that will be reported by emitting a


TestLoadFinishedEvent
event with anerrorMessage
instead of asuite
.This required some changes to how
run()
decides which tests to run. If you're interested, I can submit another PR that uses theerrorMessage
feature to report test runner failures as well instead ofCreateTestSuiteError()
, if you're interested. Conversely, if you'd rather I mimicCreateTestSuiteError()
instead for this PR, I'd be happy to make that change.