Skip to content

NUnit3Templates update - #5634

Merged
dsplaisted merged 1 commit into
dotnet:release/3.1.1xxfrom
halex2005:nunit3templates-update
Nov 26, 2019
Merged

NUnit3Templates update#5634
dsplaisted merged 1 commit into
dotnet:release/3.1.1xxfrom
halex2005:nunit3templates-update

Conversation

@halex2005

Copy link
Copy Markdown

This PR updates NUnit3.DotNetNew.Template versions with following fixes (see nunit/dotnet-new-nunit#33):

  • update Microsoft.NET.Test.Sdk dependency to v16.4.0
  • fixed condition to include latest version of Microsoft.NET.Test.Sdk for netcoreapp2 and higher

@halex2005
halex2005 force-pushed the nunit3templates-update branch from 21ef170 to fcd7640 Compare November 12, 2019 13:06
@nguerrera

Copy link
Copy Markdown

Sorry, this was missed, and I think it is too late for 3.1.100. It will have to go in 3.1.101 if we can get approval.

@halex2005 I suggest tagging @dotnet/dotnet-cli in the future.

@halex2005

halex2005 commented Nov 19, 2019

Copy link
Copy Markdown
Author

Why it is late?
Do you want to release SDK with subtle bug in nunit template?
What about quality of product?

In my opinion, this PR do not affect on SDK, but increase its quality. It just contains simple string with new version of other package, and all checks have passed btw!

@nguerrera

nguerrera commented Nov 19, 2019

Copy link
Copy Markdown

Why is it late?

We already have the presumed final build in hand. The bar is extremely high to reset that. There are factors WAY outside my control on when things can be approved. There's a work back from other dates like when the final VS 16.4 build will be and how long to insert into that, etc.

It would still have been tight, but if we had jumped on the PR when it came in, we would have had a good chance to get it in. For that not happening, I've apologized above and I'm apologizing here again. The only excuse I have is that we have a lot of repos/branches/releases going on at the same time. Feel free to ping me directly on future PRs to ensure that they are looked at sooner.

Do you want to release SDK with subtle bug in nunit template?

I do not unless there is no choice.

There are not enough details to fully understand the bug yet. Can you describe:

  1. Is this a regression from a prior SDK release? If so, when was it introduced.
  2. What is the customer impact of not taking this fix?
  3. Is there a workaround? If so, what is it?
  4. What risks are there with this change?

I will submit these answers for approval, but it is a long shot. I do not make the decisions here.

What about quality of product?
In my opinion, this PR do not affect on SDK, but increase its quality. It just contains simple string with new version of other package, and all checks have passed btw!

We all care deeply about this. Holding a high bar at the end of a release helps that more than it hurts, though individual decisions might sting. If our only bar was that PRs were green and believed to increase quality, we would definitely introduce more last minute regressions and actually lower quality of final release. Any given change can be low risk, but the cumulative risk can still be very real, given enough changes. Out of fairness, we have to apply the same bar to all the changes.

@nguerrera nguerrera added this to the 3.1.1xx milestone Nov 19, 2019
@halex2005

halex2005 commented Nov 20, 2019

Copy link
Copy Markdown
Author

@nguerrera, I understand that you have some timetable and release cycle.

I heard that 3.1.100 will be released in the beginning of december (or hopefully in the end of november), it is 10 days to release btw.
Look at PR - it is only two strings - versions of nuget packages. All the checks have passed.

This definitely will not break anything if you merge it, and you definitely should not retest all the stuff after merge. It just will contains up-to-date package versions. For me, that is just the sanity.

Btw, it is ok for me if this PR will have to go in 3.1.101. I just would like to have this PR merged.
What should I do now? Should I change target branch for PR?

PS. You said that you have the presumed final build in hand. If you will need to rebuild the final build with some fixes, please, do not forget to merge this PR :)

@nguerrera

nguerrera commented Nov 20, 2019

Copy link
Copy Markdown

I understand that you have some timetable and release cycle.

Thanks for understanding. :)

it is 10 days to release btw.

As I said, there's a work back from that. 10 days to release is not 10 days to checkin.

If you will need to rebuild the final build with some fixes, please, do not forget to merge this PR

Even if that happens, I cannot merge this without approval from a group of people that review late changes and servicing changes. To get that approval, I would need to supply the answers to my questions above, so it would be helpful if you could provide the answers. Also, the same questions would apply for 3.0101 so the answers are needed either way. Once we enter servicing, all changes are subject to this approval.

@nguerrera

Copy link
Copy Markdown

Should I change target branch for PR?

No, this is the correct branch for 3.1.101, once 3.1.100 is officially final, then changes for 3.1.101 can go into the branch.

@halex2005

Copy link
Copy Markdown
Author
  1. Is this a regression from a prior SDK release? If so, when was it introduced.

First of all, nunit dotnet new templates are works correctly. But there is subtle bug when targeting .netcoreapp >= v2 in templates - wrong condition will emit <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.2.0" /> instead of 16.4.0.

  1. What is the customer impact of not taking this fix?

Customers will update Microsoft.NET.Test.Sdk nuget package manually.

  1. Is there a workaround? If so, what is it?

Yes, there is workaround: customers could install lateset templates with dotnet new -i NUnit3.DotNetNew.Template.

  1. What risks are there with this change?

There is no risks. All will works, but Microsoft.NET.Test.Sdk package version is not the latest.

@nguerrera

Copy link
Copy Markdown

Thank you.

@dsplaisted
dsplaisted merged commit 24f420c into dotnet:release/3.1.1xx Nov 26, 2019
@halex2005
halex2005 deleted the nunit3templates-update branch November 27, 2019 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants