-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
MSTest SDK document limitations #43717
Open
Evangelink
wants to merge
4
commits into
dotnet:main
Choose a base branch
from
Evangelink:mstest-sdk-limitation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
If someone is here viewing this section, which is Known limitations, it's not really beneficial to try and use CAPS to emphasize something that is a known limitation. Someone taking the time to read this paragraph will understand the context. It's not hidden in a sea of paragraphs and tables, it's the first thing mentioned. If anything, I would use this instead:
But honestly this isn't very clear about the limitation. As a user, I don't want to have to go click on a link and read through comment-after-comment to understand the limitation. Just tell me the limitation and what I need to do to work around it.
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.
@adegeo Could you please suggest a wording. Basically, NuGet integration is busted so you cannot use VS/VSCode or NuGet CLIs to manage the update of the package and will instead need to do it manually or rely on dependabot.
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.
@Evangelink Sorry for the delay, I was out sick. If you understand the problem/limitation, can you reiterate it here in a response to myself? I'll take that info and provide a suggestion.
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.
No worries. As I was saying in the previous comment, the limitation is that you cannot use existing tooling to install or update the SDK package (VS nuget package manager won't work, dotnet nuget won't work...). You have to update manually or rely on dependabot.
This problem is not limited to MSTest but is true for any custom MSBuild sdk
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.
The dependabot suggestion seems confusing. I don't know how someone would use dependabot, I thought that was just a github thing? Like if I send a copy of my project to a friend through email, what does he do to restore the packages and run the project? How does he actually manually install the required SDK files?
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.
Ok let me list scenario:
1/ project is configured to be using MSTest SDK, you open it in VS/VS code or build it in cli then all is good. Restore is working properly, build is working you are all set.
2/ there is a new version of the SDK or some packages referenced by the SDK. You open VS nuget package manager and see only updates for the referenced packages not the SDK. When trying to update those packages they are added to your project but the SDK itself isn't updated and user can only update manually. The story is the same for VS code or cli.
3/ project isn't using MSTest SDK and you want to use it. Then you have to manually update your project files.
For the part about dependabot, yes this is GH only feature but it doesn't understand custom SDK where the version is declared into global.json so if you are using the SDK with version defined there and your code is on GH with dependabot configured, you would get bot PR bumping the version.
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.
@Evangelink I think you meant it does understand custom SDK?
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.
@Evangelink @adegeo What do you think of this wording? Does it make it clearer?