numaru / vscode-ceedling-test-adapter

Ceedling Test Adapter for the VS Code Test Explorer
MIT License
37 stars 14 forks source link

Add diagnostics (entries on problems tab) for warnings and errors #56

Closed EighthMayer closed 4 years ago

EighthMayer commented 4 years ago

Hello!

I'd like to add diagnostics reporting mechanism similar to VSCode's Tasks ProblemMatchers (unfortunately it's not possible to use them directly from extensions, as far as I know). Main idea is to parse stderr with regular expression(s) to get compiler messages for errors and warnings, and if there is any - add them to VSCode problems tab.

I'm not sure if it's worth a separate issue so I'll say it here - also for this to work better, I need to add an option to run suits with "clobber", so everything would be rebuilt on every run, and report warnings properly. Actually I think this option currently would be useful on it's own, because Ceedling sometimes fail to recognize that rebuild is necessary - for example on project.yml or mocked header changes. Edit: figured out it's already possible to do this via testCommandArgs setting.

What do you think about that?

numaru commented 4 years ago

Hello!

I don't think it should be implemented in vscode-ceedling-test-adapter. This repo is an Adapter and contains only the specific code to launch ceedling commands and parse the output. The extension is a part of a bigger project called vscode-test-explorer. The tree view and UI stuff is done there. Also from the readme.md, it looks like someone already tried your idea and published a Controller named vscode-test-explorer-diagnostics. It might suits your needs.

EighthMayer commented 4 years ago

Yes, I'm aware about the infrastructure. vscode-test-explorer-diagnostics is not currently suitable because it just duplicates info about failed tests to problems tab. Owner of vscode-test-explorer-diagnostics is not interested in adding what I spoke of. She suggested to pass warnings and errors as decorations along with test events, which is still requires parsing of output elsewhere and thus not seems like the best solution (I mean, if we already have diagnostics, we could as well just emit them, no need to pass them somewhere else to do just the same thing).

Well yes, maybe it should be in a separate controller (however, I still need modifications in adapter for this - stderr needs to be passed in message no matter the state of test, or I won't get warnings). I just thought it would be handy to have everything related in the same place. But now when I think about it - yes, Ceedling is separate from compiler and linker it using, so it would be a bit strange to handle compiler/linker output in strictly Ceedling-related piece of code.

Perhaps I should make a new controller, which would parse messages of test events? How about passing stderr to message field?

numaru commented 4 years ago

My bad, I initially missed your point and the fact it already has been discussed in a diagnostics repository issue. I get it now and I think it is a good idea 😃. Since what you want to achieve is related to the way ceedling works, it uses its own build system, we should implement the parsing in the adapter extension.

I don't know if another adapter would be interested by the diagnostics and if it should be represented in the test explorer api. So I agree to the implementation of both the parsing and the VSCode diagnostics in the adapter for now.

EighthMayer commented 4 years ago

Actually it's my bad, I should have mentioned the prior conversation with the owner of diagnostics controller.

I'm still not sure which approach is better - to make a separate vscode-test-explorer-diagnostics-messages controller or to implement parsing in the adapter. Both have pros and cons. But I guess it would work better if implemented in the adapter and maybe would take less effort to maintain in the future due to smaller scope. So if you're Ok with that - I'll tinker with it and would show what I've got later. Thank you.

EighthMayer commented 4 years ago

Hello! I've made a pull request #63 for this issue.

Have a great day!

EighthMayer commented 4 years ago

By the way - it is disabled by default. To make it enabled by default (in case you would want it):

  1. Set default value of ceedlingExplorer.problemMatching.mode to "gcc"
  2. Modify README.md accordingly (lines 16, 52..61)

Edit: simplified settings

numaru commented 4 years ago

Implemented in the PR.