Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 feature flag to allow VS to use the LSP based editor #49996
Add feature flag to allow VS to use the LSP based editor #49996
Changes from 20 commits
a4d4605
97c54a2
a16ad35
2a9b7e4
f7444f7
35e35d8
c10d1cd
34d355d
2b0f6a6
8421485
ce45c40
1b9d87a
cdad03e
2a27f8a
66c87a3
692c6fc
20485ba
795ab89
36d87a6
e34dbb9
7ccdf64
d0305d9
75f8c04
b3288f4
f58231c
38179be
edf1c86
39423e8
a894943
e1fa552
68b9691
bdb3aa5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I went with both a separate namespace and additional trait name to enable multiple testing scenarios:
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.
I guess the trait is sufficient right, if you just put the trait on containing types? My strong worry about the magic namespace is that we might rename or move something and not realize that tests aren't running anymore. Having the single method with the trait makes it a bit easier to know everything either works or doesn't.
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.
(see other comment though that the only test currently in it's own namespace I think is better just combined with the existing test which checks if it's in LSP mode or not...)
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.
And yes, we once had an integration test CI that was passing because whatever glob was looking for assemblies to run wasn't updated, and so it was running nothing. That was a "fun" discovery.
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.
@jasonmalinowski If we ever want to support running a test in LSP only context then we must have a separate namespace or something similar. Otherwise I have no way to filter out LSP-only tests from the normal run (since the normal run includes shared tests with the LSP trait).
I can guarantee that there will be LSP only tests, if not gotodef then completion or another feature. I think even for the gotodef test you mention I must have an LSP version and a non-LSP version, even if I share most of the code as they have different input data (window caption).
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.
I guess as long as we're using environment variables to control this, there's no reason you can just haven't a ConditionalFact that only runs if the environment variable is set. You can also have the test just check the environment variable in a test if you need to change a string.
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.
Ah! Today I learned about conditional facts. Ok, I'll use that in a followup instead.
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.
fyi, i ignored all the yaml/ci stuff. someone else will have to review that.
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.
As best as I could tell, the trait and notrait properties did not work properly with dotnet test. They would pass an argument like Trait=LanguageServerProtocol, but per https://docs.microsoft.com/en-us/dotnet/core/testing/selective-unit-tests?pivots=xunit that isn't the correct format. So I modified these to be able to just pass in the xunit test filter (which also allowed me to pass in fully qualified names)
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.
is it possible to extract this out into anotehr pR?
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.
i skipped all this.