dotnet / scenario-tests

Scenario testing for installed .NET Core SDKs
MIT License
5 stars 7 forks source link

Changes Downlevel tests to MultiTFM tests and adds helper functions to support #26

Closed mattscheffer closed 12 months ago

mattscheffer commented 1 year ago

@mmitche Could you please review this PR? Thanks!

mmitche commented 1 year ago

Yep will do.

mattscheffer commented 12 months ago

I think that rather than adding the "AddMultiTfm" flag, you could replace the framework parameter here https://github.com/dotnet/scenario-tests/pull/26/files#diff-d73f217aafa6da3fc7d51af898445b94f311c19f4577a91011c9ab194a3b622cR26 (, string? framework = null) as "string[] additionalFrameworks = null". If that is non-empty, then add the additional tfms specified.

That parameter then gets passed to ExecutePublish, ExecuteRun, etc. where you loop over the additionalFrameworks and execute the commands for those frameworks, in addition to the default framework.

@mmitche I've changed some of these locally. However quick question. Are we planning on having more than one set of frameworks for multitfm scenarios? Or adding a parameter so that the user can select what runtimes they have? If not, then we have no reason not to have a hardcoded string with the test and subsequently not much reason to complicate things with loops rather than a conditional.

mmitche commented 12 months ago

I think that rather than adding the "AddMultiTfm" flag, you could replace the framework parameter here https://github.com/dotnet/scenario-tests/pull/26/files#diff-d73f217aafa6da3fc7d51af898445b94f311c19f4577a91011c9ab194a3b622cR26 (, string? framework = null) as "string[] additionalFrameworks = null". If that is non-empty, then add the additional tfms specified. That parameter then gets passed to ExecutePublish, ExecuteRun, etc. where you loop over the additionalFrameworks and execute the commands for those frameworks, in addition to the default framework.

@mmitche I've changed some of these locally. However quick question. Are we planning on having more than one set of frameworks for multitfm scenarios? Or adding a parameter so that the user can select what runtimes they have? If not, then we have no reason not to have a hardcoded string with the test and subsequently not much reason to complicate things with loops rather than a conditional.

I dont' see a reason yet to need outside input. Since there were two places where we were dealing with the input TFMs, I would extract that into a static string[] array somewhere, rather than hardcoding into the test logic.

mattscheffer commented 12 months ago

I think that rather than adding the "AddMultiTfm" flag, you could replace the framework parameter here https://github.com/dotnet/scenario-tests/pull/26/files#diff-d73f217aafa6da3fc7d51af898445b94f311c19f4577a91011c9ab194a3b622cR26 (, string? framework = null) as "string[] additionalFrameworks = null". If that is non-empty, then add the additional tfms specified. That parameter then gets passed to ExecutePublish, ExecuteRun, etc. where you loop over the additionalFrameworks and execute the commands for those frameworks, in addition to the default framework.

@mmitche I've changed some of these locally. However quick question. Are we planning on having more than one set of frameworks for multitfm scenarios? Or adding a parameter so that the user can select what runtimes they have? If not, then we have no reason not to have a hardcoded string with the test and subsequently not much reason to complicate things with loops rather than a conditional.

I dont' see a reason yet to need outside input. Since there were two places where we were dealing with the input TFMs, I would extract that into a static string[] array somewhere, rather than hardcoding into the test logic.

While I can do so, we're not using input TFM's elsewhere. Those were from the downlevel tests that were changed to multi-tfms. Unless you mean the web app tests which are commented out because we aren't installing ASP.net runtimes.

mmitche commented 12 months ago

I think that rather than adding the "AddMultiTfm" flag, you could replace the framework parameter here https://github.com/dotnet/scenario-tests/pull/26/files#diff-d73f217aafa6da3fc7d51af898445b94f311c19f4577a91011c9ab194a3b622cR26 (, string? framework = null) as "string[] additionalFrameworks = null". If that is non-empty, then add the additional tfms specified. That parameter then gets passed to ExecutePublish, ExecuteRun, etc. where you loop over the additionalFrameworks and execute the commands for those frameworks, in addition to the default framework.

@mmitche I've changed some of these locally. However quick question. Are we planning on having more than one set of frameworks for multitfm scenarios? Or adding a parameter so that the user can select what runtimes they have? If not, then we have no reason not to have a hardcoded string with the test and subsequently not much reason to complicate things with loops rather than a conditional.

I dont' see a reason yet to need outside input. Since there were two places where we were dealing with the input TFMs, I would extract that into a static string[] array somewhere, rather than hardcoding into the test logic.

While I can do so, we're not using input TFM's elsewhere. Those were from the downlevel tests that were changed to multi-tfms. Unless you mean the web app tests which are commented out because we aren't installing ASP.net runtimes.

I'm just talking about the usages here https://github.com/dotnet/scenario-tests/pull/26/files#diff-6f10b5158aaa5a71f99abbc13de92c6f8c2510ae28edfab51fbef19fd435918fR139-R147, https://github.com/dotnet/scenario-tests/pull/26/files#diff-6f10b5158aaa5a71f99abbc13de92c6f8c2510ae28edfab51fbef19fd435918fR161-R163, https://github.com/dotnet/scenario-tests/pull/26/files#diff-6f10b5158aaa5a71f99abbc13de92c6f8c2510ae28edfab51fbef19fd435918fR195-R209 and https://github.com/dotnet/scenario-tests/pull/26/files#diff-6f10b5158aaa5a71f99abbc13de92c6f8c2510ae28edfab51fbef19fd435918fR297

When new TFMs are added and we want new downlevel targets, we will end up having to update 4 places. We can make that 1 instead by utilizing a static array of strings and loops/string.join for the last case.

mattscheffer commented 12 months ago

@mmitche I've done as asked. Frameworks are only coded in at one string array. Loops are used for the other points