Skip to content

Add sharedfx and targeting pack tests (#23045) #23070

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

Merged
merged 4 commits into from
Jul 15, 2020

Conversation

JunTaoLuo
Copy link
Contributor

Addresses #19151 and #14832. Backport of #23045

* Add test for assembly versions

* Add test for framework list

* Add some hardcoded lists for sharedfx and targeting pack content
@JunTaoLuo JunTaoLuo added the tell-mode Indicates a PR which is being merged during tell-mode label Jun 18, 2020
@ghost ghost added the feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform label Jun 18, 2020
@JunTaoLuo JunTaoLuo requested a review from a team June 18, 2020 00:25
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Please test the targeting pack part of this offline before merging, if you haven't already. Not building them right now.

And, to confirm: The hard-coded lists of expected assemblies isn't quite identical to what we're looking for in 'master', correct❔

@JunTaoLuo
Copy link
Contributor Author

Yea I'll be tweaking a few of the details before merging.


if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
listedSharedFxAssemblies.Remove("aspnetcorev2_inprocess");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm now I'm wondering why this test didn't fail in master without adding this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah it's probably because it's testing the targeting pack that was built on Windows, given that we run the test on Helix. It should be benign then since we only ship the package built on Windows anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is testing the shared framework and we ship that for every platform.

  1. Should not muck with the ListedSharedFxAssemblies value in a single test. This means other tests may fail depending on test execution order. The TestData value itself should conditionally exclude the native assembly.
  2. It's not clear how this test is passing on non-Windows platforms in 'master'. Please figure that out.

Separately, this won't work unless you also exclude the ANCM assembly on Windows ARM and ARM64. Probably better to copy the $(VerifyAncmBinary) property and associated TestData from 'master' to handle future testing on those platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. I'll address 1. As for 2, the reason is that we are running the tests on Helix and we use the test payload that was built on Windows even if we run the tests on non-Windows. @HaoK do we have infrastructure to build test payloads on non-Windows OS's. I don't remember having anything like that so maybe we should be running the tests on AzDO until we can do that?

Copy link
Member

Choose a reason for hiding this comment

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

We don't have infrastructure to build on non windows yet, so maybe this test should have SkipTests explicitly set to false so it runs on azdo as well, since the behavior is different.

Copy link
Member

Choose a reason for hiding this comment

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

These seem like a good candidate for tests that we just never run on helix too, if they are testing installer/platform specific things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make sure the tests run when I merge this forward to master.

@JunTaoLuo
Copy link
Contributor Author

Tested locally to ensure the tests would pass when run. Made a few changes that warrant another look.

@JunTaoLuo JunTaoLuo requested a review from dougbu June 18, 2020 07:58
@wtgodbe wtgodbe added this to the 3.1.7 milestone Jun 18, 2020
@JunTaoLuo JunTaoLuo requested a review from a team June 18, 2020 17:30

if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
listedSharedFxAssemblies.Remove("aspnetcorev2_inprocess");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is testing the shared framework and we ship that for every platform.

  1. Should not muck with the ListedSharedFxAssemblies value in a single test. This means other tests may fail depending on test execution order. The TestData value itself should conditionally exclude the native assembly.
  2. It's not clear how this test is passing on non-Windows platforms in 'master'. Please figure that out.

Separately, this won't work unless you also exclude the ANCM assembly on Windows ARM and ARM64. Probably better to copy the $(VerifyAncmBinary) property and associated TestData from 'master' to handle future testing on those platforms.

@JunTaoLuo
Copy link
Contributor Author

🆙📅

@JunTaoLuo
Copy link
Contributor Author

@wtgodbe can go into release/3.1 right now? I just want to double check it wouldn't interfere with any of your 3.1 work.

@wtgodbe
Copy link
Member

wtgodbe commented Jun 19, 2020

@JunTaoLuo please wait for next month's release, the build for 3.1.6 is already pretty much done

@JunTaoLuo JunTaoLuo merged commit d101de8 into release/3.1 Jul 15, 2020
@JunTaoLuo JunTaoLuo deleted the johluo/add-framework-tests branch July 15, 2020 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants