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
Convert fallback path of GetCommandLineArgs to managed #70608
Convert fallback path of GetCommandLineArgs to managed #70608
Changes from 2 commits
39d2f90
bc6540a
9711a33
e36103c
aaa5def
03d582a
a3b3db5
01eeb9e
4ff89e1
9ebedfe
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 would prefer this to follow the
[Fact]
pattern we are moving towards.Removing the namespace, making the class
public
, renaming toCommandLineTest
and then renamingint Main()
as follows should be all that is needed. Also, removing the return as this will all be generated for you.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.
This test does not really need to be under src/tests. It can be under libraries (and using remote executor). It is a test for
Environment.GetCommandLineArgs()
library API after all.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 considered that under xunit we may have much less control of the actual command line of the process. Can xunit be run in-process by Visual Studio?
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.
@AaronRobinsonMSFT What's your thought about the expected value here? I'm afraid the only thing we can test is not empty, which looks not enough.
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.
In general, I agree. However, the libraries teams have traditionally made an effort to call every API even if it is only to verify it always throws or returns nothing. It does make for some odd tests, but is helpful in ensuring the API works enough to invoke. More to the point, I think it is already done https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime.Extensions/tests/System/Environment.GetCommandLineArgs.cs.
Once I update CoreShim to be more useful we can create the more robust suite I think we all want.
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.
This needs to be marked
static
, but I'd prefer making it aFact
.