Skip to content
This repository was archived by the owner on Apr 20, 2023. It is now read-only.

Vstest honor nologo#9791

Closed
mayankbansal018 wants to merge 1 commit intodotnet:release/2.1.4xxfrom
mayankbansal018:vstest-honor-nologo
Closed

Vstest honor nologo#9791
mayankbansal018 wants to merge 1 commit intodotnet:release/2.1.4xxfrom
mayankbansal018:vstest-honor-nologo

Conversation

@mayankbansal018
Copy link

@mayankbansal018 mayankbansal018 commented Aug 3, 2018

Passing /nologo user input to VSTest task

Related: microsoft/vstest#1717

Bug: microsoft/vstest#1701

@mayankbansal018 mayankbansal018 changed the title [WIP]Vstest honor nologo Vstest honor nologo Aug 3, 2018
@mayankbansal018
Copy link
Author

@smadala , for review

@mayankbansal018
Copy link
Author

@dotnet-bot please test Ubuntu x64 Release

@mayankbansal018
Copy link
Author

@dotnet-bot test this please

}

// Add VSTestNoLogo property if specified by user
if(parsedTest.Arguments.Contains("--nologo") || parsedTest.Arguments.Contains("/nologo"))

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@mayankbansal018
Copy link
Author

@livarcocc , can you please merge this PR

Copy link

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Can we add a unit test to cover the suppression of VSTest's logo when --nologo is passed to dotnet test?

@mayankbansal018
Copy link
Author

@peterhuene , I will add unit tests, once we have inserted our latest sdk, which supports this. Do we want to wait till then to merge the PR?

@peterhuene
Copy link

Hi @mayankbansal018,

Sorry, this fell of my radar when I was on vacation. Since 2.1.4xx has shipped and this might not meet the 2.2.100 bar at this point, could you retarget this PR at master (i.e. 3.0)?

Additionally, I'd hold off merging this until you have a test SDK inserted (or can do so as part of this PR) so we can have a test added. It's fine to also add the test now for review purposes and have the PR CI fail, which can then be rerun once the test SDK is inserted.

@peterhuene
Copy link

Hi @mayankbansal018. I just wanted to follow up and make sure we retarget this PR against master. We should be able to easily insert a new test SDK there as well. Thanks!

@peterhuene
Copy link

Hi @mayankbansal018,

I just wanted to follow-up one more time so that we don't lose track of this PR. Is this still something the test team wants to move forward with for 3.0?

Thanks!

@mayankbansal018
Copy link
Author

Thanks @peterhuene , for keeping track of this, unfortunately this slipped from my mind. I'll raise a new PR against appropriate branch.

@livarcocc
Copy link

@livarcocc
Copy link

Closing in favor of #10662.

@livarcocc livarcocc closed this Jan 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants