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

Fixes Surround ($IsPackable) with single quotes #4159

Merged
merged 1 commit into from
Aug 14, 2021

Conversation

eriawan
Copy link
Contributor

@eriawan eriawan commented Jul 20, 2021

Bug

Fixes: NuGet/Home#11025

Regression? Last working version:

Description

Fixes NuGet/Home#11025 of having IsPackable not surrounded by single quote

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes: This PR is surrounding $(IsPackable) with single quotes to fix log as mentioned in linked issue

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@eriawan eriawan requested a review from a team as a code owner July 20, 2021 22:57
@ghost ghost added the Community PRs created by someone not in the NuGet team label Jul 20, 2021
@eriawan eriawan changed the title Fixes Surround $($IsPackable) with single quotes Fixes Surround ($IsPackable) with single quotes Jul 20, 2021
@eriawan
Copy link
Contributor Author

eriawan commented Jul 21, 2021

The CI check has failed. As far as I know, I think this PR has no breaking changes.

Since the CI check is not public, is there any other way to know what exactly failed the CI? Or do I have to update this PR to fix the fail?

cc @nkolev92 @erdembayar

@erdembayar
Copy link
Contributor

erdembayar commented Jul 21, 2021

The CI check has failed. As far as I know, I think this PR has no breaking changes.

Since the CI check is not public, is there any other way to know what exactly failed the CI? Or do I have to update this PR to fix the fail?

cc @nkolev92 @erdembayar

Thank you for your contribution.
It looks it's flaky test failing, I requeued the test. Update: All existing tests are passing.
Also could you be able to add unit tests to assert if problem is solved and it wouldn't happen again. Possible places where new unit test can go are:
https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetPackCommandTest.cs
https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs
https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Core.Tests/NuGet.Build.Tasks.Pack.Test/PackTaskTests.cs

@eriawan
Copy link
Contributor Author

eriawan commented Jul 22, 2021

@erdembayar

Thanks for the CI check!
I had checked all of the three places you mentioned, and I think I need more info, because to test this PR means I'm going to compare the result of MSBuild, to reproduce the scenario in the original bug, correct me if I'm wrong 🙂
Therefore, could you have sample existing unit test that check the log from MSBuild to be tested? Or do those three unit test cs files have some samples for me to look and start write tests?
That would be helpful for me to start writing the unit test tomorrow, as you have suggested to test and assert my PR.

@erdembayar
Copy link
Contributor

erdembayar commented Jul 23, 2021

@erdembayar

Thanks for the CI check!
I had checked all of the three places you mentioned, and I think I need more info, because to test this PR means I'm going to compare the result of MSBuild, to reproduce the scenario in the original bug, correct me if I'm wrong 🙂
Therefore, could you have sample existing unit test that check the log from MSBuild to be tested? Or do those three unit test cs files have some samples for me to look and start write tests?
That would be helpful for me to start writing the unit test tomorrow, as you have suggested to test and assert my PR.

There should be existing sample unit test which you can modify or create new ones from it. Please check existing tests first, if you have problem locating one close to your need then please let me know. Please check contributing guide for how to jump start.

@nkolev92 nkolev92 assigned nkolev92 and unassigned nkolev92 Aug 14, 2021
@nkolev92
Copy link
Member

there's no functional issue here. So I don't think a test is necessary.

@nkolev92 nkolev92 merged commit 352ba94 into NuGet:dev Aug 14, 2021
@nkolev92 nkolev92 self-assigned this Aug 14, 2021
@eriawan
Copy link
Contributor Author

eriawan commented Aug 14, 2021

@nkolev92
thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
3 participants