Skip to content
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 test explorer #893

Merged
merged 13 commits into from
Mar 16, 2022
Merged

Add support for test explorer #893

merged 13 commits into from
Mar 16, 2022

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Mar 12, 2022

Following up on #796, I'm trying to address the remaining work.
@baronfel, I changed the parameter type to a new type, but I'm guessing there is a bit more to it.
Could you point me in the right direction on what I need to do with TestDetectedNotification?

@baronfel
Copy link
Contributor

@nojaf what I meant by a new type is that the PlainNotification type is serialized and sent to VSCode (and other clients) as a { Content: string } structure, and I'd much prefer if instead it could be sent with a more useful structure. We already have a useful structure for the message and I would much rather send this structure in the notification message (letting newtonsoft.json serialize it to json) than sending a raw string to the client.

Essentially, IMO we should be reducing the amount of usage of PlainNotification in favor of typed responses all the time, and integrating that into callsites in Ionide as we change that contract.

@nojaf
Copy link
Contributor Author

nojaf commented Mar 15, 2022

@baronfel this first test worked on my machine.
I'm not really familiar with Expecto.
Please let me know if this is more or less what you had in mind.

@baronfel
Copy link
Contributor

These tests are exactly what I had in mind! As we expand detection support we can of course flesh them out 👍

@baronfel
Copy link
Contributor

This is great - the failing windows tests are the flaky ones that are not related to this effort. I'm good to merge this. Once it merges we can look at the Ionide side of things (which will likely need a slight tweak as the shape of the new notification has changed slightly from the original pass of this work).

@baronfel baronfel merged commit 20863e2 into ionide:main Mar 16, 2022
@nojaf nojaf deleted the test-explorer branch March 16, 2022 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants