Issue 183: use the relative root cargo file path if set by the user

[EricHenry]

Summary

#183

Previously, the extension setting rootCargoManifestFilePath was not being used to locate the relative root file path for a cargo file if it was set by the user. This PR now looks for the set configuration and appends it to the workspace file path.

[codecov[bot]]

Codecov Report

Merging #186 (09c0d0d) into master (8948c5d) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #186   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          413       412    -1     
  Branches        57        53    -4     
=========================================
- Hits           413       412    -1     

Impacted Files	Coverage Δ
src/test-loader.ts	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 55de9de...09c0d0d. Read the comment docs.

[calebcartwright]

Will take a look at this one tomorrow. Don't worry about that sonar job failing in CI, we need to change the configuration to not run against forks due to the associated secret being unavailable

[calebcartwright]

Thanks again for the PR!

I confess I'm a little torn on if/how to proceed. Unfortunately much of the plumbing for ingesting configuration settings, as well as subscribing to configuration change events, is still missing. Similarly, very little of the internals are wired up for utilizing said configuration.

Technically what you've done here can work but has some marked limitations due to the above constraints, notably:

• Users have to restart VS Code for the change to take effect
• Users must provide the directory containing the Cargo manifest, not the path to the toml file, because the arg is really for the root directory and not a value that would be passed to cargo commands via the --manifest-path option.

Since the current option is essentially a no-op, I think I can get on board with this despite those limitations, provided we also document them accordingly to avoid a similar scenario as the current state.

Alternatively, if you're up for trying to to implement the more holistic option that'd be a massive help and I'm happy to provide some pointers to outline the approach.

[calebcartwright]

Also relevant to considerations around additional investment/enhacnement is that the Test Explorer extension framework has actually been deprecated for a while now since VS Code added a native Test API.

I've been holding off on making the switch as it'll likely be a pretty heavy refactor, and there's been some conversations about adding Test Explorer-like functionality natively to Rust Analyzer which I hope to see come to fruition

[EricHenry]

That makes a ton of sense. I would be willing to take a crack at a more holistic approach, given your guidance. Admittedly, this was very much a get it working PR.

However, I understand if you are cautious about any heavy refactors given the potential for test explorer functionality going into Rust Analyzer.

I'll defer to your best judgement of the situation. I would be happy to help anywhere I can. Whether that is implementing a more holistic approach, helping migrate to VSCode's native test API, or just providing more documentation around the current changes.

Thanks for all the insight!

[calebcartwright]

Awesome thank you. I need to refresh my memory a bit, but will try to lay out a suggested path over the next couple days

[calebcartwright]

I look at the work needed for this in two separate buckets, (1) is actually wiring up configuration settings and (2) is utilizing the config in relevant places in order to use this particular option.

For (1):

• Config interface/type (I believe I was planning on using this interface but it's been a while and I might be remembering wrong :sweat_smile:). I'd think we'd want to add a field there for the root manifest path (ignore the other settings for now)
• ATM the adapter is just generating a hard coded instance of the config (https://github.com/swellaby/vscode-rust-test-adapter/blob/55de9de71f8d8c58c5e27320b68e19d1c68cc5e4/src/rust-adapter.ts#L54 Believe we'll need to update the adapter's constructor to accept a config (just make sure we keep loadUnitTests enabled, hardcoded is fine) and then I imagine we'll need to make that config a class member
• In order to be able to run true unit tests against the logic, we encapsulate the vscode imports as much as possible (because it's not possible to unit test a module that imports vscode) which is why there's a separation between the entry point module and the main adapter class. However, we'll need the extension to hook into VS Code's configuration change event in order to be able to pick up changes without forcing the user to restart VS Code every time. I'm definitely open to any alternatives but I think the quickest path to supporting this is to set up a subscription (onDidChangeConfiguration) for that event and then to get that propagated to the adapter class instance. Not 100% sure what the best way to do that will be given the registration flow, but can envision a couple different options

For (2):

• Basically just need to update relevant fn signatures to ensure they also accept an IConfiguration param and then use it when/where appropriate.

Think we'd want to conditionally include a --mainfest-path ${config.rootManifestPath} arg when present, and if not resort to running commands in the workspace root

https://github.com/swellaby/vscode-rust-test-adapter/blob/55de9de71f8d8c58c5e27320b68e19d1c68cc5e4/src/cargo.ts#L52-L68