-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
resolve problemMatchers for tasks executed by vscode.tasks.executeTask api call #9207
Conversation
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.
@vnfedotov thank you for your first contribution! Do you mind squashing your commits and we will proceed with a review 👍
cc @alvsan09 would you like to review?
…k api call Signed-off-by: Vladimir Fedotov <[email protected]>
Sure @vince-fugnitto, I will review it as well !! |
HI @vnfedotov, |
Sure, @alvsan09 Observed behaviour before change:
Observed behaviour after this change:
|
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.
Hello!
Thank you for your contribution!
I think the current approach works well except the use case when a user has configured a task and applied another problem matcher.
For example, for a typescript
task tsc
problem matcher is used. But a user can configure the task and set another problem matcher or an empty array for the problemMatcher
field (like, Never scan the task output
). At running the configuration that value from tasks.json
config file is used.
For your use case it's possible to configure the test
task and override the problemMatcher
filed in the tasks.json
config file, but looking on the provided changes I think that task system will always use original problemMatcher
from the task configuration.
Is it expected behavior?
Could you test the use case on VS Code side:
- configure your
test
task and set another value for theproblemMatcher
field or an empty array - run
test
task from theTest view
- detect somehow which
problemMatcher
is used - the new value or the original value provided by your plugin.
Thank you!
@RomanNikitenko I believe it's done already as it is. The reason is: configured tasks (defined in tasks.json) use different TaskService entrypoint, they're started with TaskService.run which then proceeds to construct TaskCustomization object and pass it as RunOptions to TaskService.runTask. Actually, it's the reason for the behavior described as "before change": problemMatchers were working when started from the menu, and were not when started from the vscode api call. A call to vscode.tasks.executeTask runs TaskService.runTask directly, without looking into tasks.json, as it should, but also loses any information on problemMatchers, as it definitely should not. I've updated https://github.com/vnfedotov/problemMatcher-test-repo to illustrate my point. I've added second problem matcher "typescript" and another line into test script that should match typescript regexp. Now it behaves as follows:
Kapture.2021-03-18.at.16.43.13.mp4 |
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.
@vnfedotov
you are right!
I tested both cases according to #9207 (comment)
and it works as expected for me!
I detected a bug: there are 2 items with the label test
after configuring your task
But I believe it's not related to your changes - something wrong with Configure Task
action.
@vince-fugnitto |
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.
@vnfedotov I noticed there are errors when attempting to run the test task:
task ERROR Error resolving task 'test': TypeError: Cannot read property 'group' of undefined
test-error.mp4
This however does not seem to be an issue with the pull-request but rather #9189. I confirmed it works before that commit.
Hey guys, are there any critical blockers preventing to merge? I've updated the changed function to get rid of the conflicts, so it's no longer affected by previously introduced parallel tasks locking feature. Could you please take a look at #10166? |
The changes were merged within #10166 |
What it does
Fixes #9205
Tasks executed with the direct call to runTask method (as it is in vscode.tasks.executeTask implementation) have no RunOptions, which also means they lose any ProblemMatchers that might be associated with this task.
Here we recreate RunOptions, resolving ProblemMatchers if they exist in TaskConfiguration.
How to test
Review checklist
Signed-off-by: Vladimir Fedotov [email protected]