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

Adding several new tests, add reference support, fixing additional ar… #11

Merged
merged 11 commits into from
Aug 25, 2023

Conversation

mattscheffer
Copy link
Contributor

…guments

@mattscheffer
Copy link
Contributor Author

So, it looks like all the tests are failing because it's actually running the tests I have here.... and that includes the downlevel tests. Apparently, .Net6 and .Net7 aren't on the test machine, so it breaks.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=378852&view=logs&j=1f132584-ab08-5fbf-e5d0-5685de454342&t=1ce98a45-1f3f-5914-0114-7502bfbbe8a4&l=106

@mmitche
Copy link
Member

mmitche commented Aug 21, 2023

@mattscheffer We can change around the global json so that it installs the downlevel runtimes. I'll work on that.

@mmitche
Copy link
Member

mmitche commented Aug 21, 2023

@mattscheffer #19

@@ -35,7 +35,7 @@ public void VerifyConsoleTemplate(DotNetLanguage language)
[Theory]
[MemberData(nameof(GetLanguages))]
[Trait("Category", "Offline")]
public void VerifyOfflineConsoleTemplate(DotNetLanguage language)
public void VerifyConsoleTemplate(DotNetLanguage language)
{
var newTest = new SdkTemplateTest(
nameof(SdkTemplateTests) + "Offline", language, _scenarioTestInput.TargetRid, DotNetSdkTemplate.Console,
Copy link
Member

Choose a reason for hiding this comment

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

"nameof(SdkTemplateTests) + "Offline" should be refactored accordingly as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack. Yes it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest commit

@mattscheffer
Copy link
Contributor Author

@mmitche It doesn't look like your fix worked. The test machines have failures at the same point.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=382650&view=results

@mmitche
Copy link
Member

mmitche commented Aug 23, 2023

@mmitche It doesn't look like your fix worked. The test machines have failures at the same point.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=382650&view=results

Looking.

@mmitche
Copy link
Member

mmitche commented Aug 23, 2023

@mattscheffer I did some poking around. Installing the downlevel runtimes will not enable downlevel targeting when using dotnet new. You actually need the downlevel SDK installed. I think this scenario, while important, is out of scope for these tests since it's using the downlevel SDK for dotnet new with targeting the older tfm, instead of the SDK being tested.

On the other hand, multi-targeting in general would work. So if you had an existing console app targeting net7, and the code actually works on net6, enabling an additional net6 TargetFramework should work.

@mattscheffer
Copy link
Contributor Author

mattscheffer commented Aug 23, 2023

@mattscheffer I did some poking around. Installing the downlevel runtimes will not enable downlevel targeting when using dotnet new. You actually need the downlevel SDK installed. I think this scenario, while important, is out of scope for these tests since it's using the downlevel SDK for dotnet new with targeting the older tfm, instead of the SDK being tested.

On the other hand, multi-targeting in general would work. So if you had an existing console app targeting net7, and the code actually works on net6, enabling an additional net6 TargetFramework should work.

@mmitche Gotcha, I was under the impression from the scenario list that this first bit was want we wanted but I can see it being out of scope, as we only care about the 1 scenario.

Do you want me to remove/comment these tests right now for this check in? Or wait to check in until multitfm is added. I'm good either way.

@mmitche
Copy link
Member

mmitche commented Aug 23, 2023

I would comment those tests for now.

@mattscheffer
Copy link
Contributor Author

Ok. Looking at the new failures. Looks like we need the skip based on platform already. WPF doesn't work at all on Linux/Mac which is causing the majority of the failures. I'll start working on that.

@@ -0,0 +1,29 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

I would move this into the Common dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved in latest commit.

using System.Runtime.InteropServices;
using System.Collections.Immutable;
using Microsoft.DotNet.ScenarioTests.SdkTemplateTests;
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary if you move the OperatingSystemFinder in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Member

@mmitche mmitche left a comment

Choose a reason for hiding this comment

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

Move the OS finder file around and should be good to go.

@mattscheffer
Copy link
Contributor Author

@mmitche Can you merge? I don't have access to do so.

@mmitche mmitche merged commit 8af694a into dotnet:main Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants