Fixing removal of None from Page under certain conditions

[rladuca]

Addresses #1758

Fixing removal of None from Page under certain conditions (EnableDefaultApplicationDefinition or EnableDefaultPageItems is false).

Further adding a fix to ensure this doesn't result in ApplicationDefinition being included in Page as well when EnableDefaultApplicationDefinition is false.

@nguerrera Can you take a look at this as well? Thanks!

[nguerrera]

@dsplaisted

[rladuca]

@nguerrera @dsplaisted Any input on this?

[dsplaisted]

Looks good, though I have a question about the last bit.

Also, I would really love to see test coverage of the different cases here.

[vatsan-madhavan]

@rladuca, Marking as milestone=5.0 since this targets master. We need another PR targeting release/3.0

[rladuca]

Looks good, though I have a question about the last bit.

Also, I would really love to see test coverage of the different cases here.

That's fair, we'd have to add this via manual tests on the backend since we won't have access to a full up SDK with these enabled. We can probably get this working by replacing the files in the builds, but for now, a manual set should suffice.

I am creating a set of tests that can easily be run for various switch combinations, I'll put them up on GitHub and we can use them in validations going forward.

[vatsan-madhavan]

I am creating a set of tests that can easily be run for various switch combinations, I'll put them up on GitHub and we can use them in validations going forward.

It's possible to re-use the tests (the infrastructure portion) from dotnet/sdk (or pehraps it is from dotnet/core-sdk) by consuming them as NuGet package and bootstrap testing for WindowsDesktop SDK. It's the sort of thing we should do in .NET 5/master (and not create a disparate test-framework/system for this).

[rladuca]

I am creating a set of tests that can easily be run for various switch combinations, I'll put them up on GitHub and we can use them in validations going forward.

It's possible to re-use the tests (the infrastructure portion) from dotnet/sdk (or pehraps it is from dotnet/core-sdk) by consuming them as NuGet package and bootstrap testing for WindowsDesktop SDK. It's the sort of thing we should do in .NET 5/master (and not create a disparate test-framework/system for this).

Yeah, that makes complete sense in the long term. The tests I have are basic project builds that I scripted to test these changes myself. I'll give these to our test team for manual verification for immediate previews and daily builds so we have some coverage until we can get a real infrastructure set up and running.

[rladuca]

I uploaded test cases I used locally to verify this change (via replacing the relevant code in a local install). They can be found here. If you have any suggestions on extra coverage or different switch combinations, let me know.

I'll give these to our test team with instructions to run against our daily builds once these changes make it to the SDK.