Skip to content

[WIP] Run helix tests on master #6728

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 44 commits into from
Feb 7, 2019
Merged

[WIP] Run helix tests on master #6728

merged 44 commits into from
Feb 7, 2019

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Jan 15, 2019

17 Skipped projects:

  • /src/Azure/AzureAD/test/FunctionalTests/Microsoft.AspNetCore.Authentication.AzureAD.FunctionalTests.csproj
  • /src/Components/test/E2ETest/Microsoft.AspNetCore.Components.E2ETests.csproj
  • /src/DefaultBuilder/test/Microsoft.AspNetCore.FunctionalTests/Microsoft.AspNetCore.FunctionalTests.csproj
  • /src/Hosting/test/FunctionalTests/Microsoft.AspNetCore.Hosting.FunctionalTests.csproj
  • /src/Identity/test/Identity.FunctionalTests/Microsoft.AspNetCore.Identity.FunctionalTests.csproj
  • /src/Middleware/CORS/test/FunctionalTests/FunctionalTests.csproj
  • /src/MusicStore/test/MusicStore.E2ETests/MusicStore.E2ETests.csproj
  • /src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/Microsoft.AspNetCore.Mvc.FunctionalTests.csproj
  • /src/Mvc/test/Mvc.Analyzers.Test/Mvc.Analyzers.Test.csproj
  • /src/Mvc/test/Mvc.Api.Analyzers.Test/Mvc.Api.Analyzers.Test.csproj
  • /src/ProjectTemplates/test/ProjectTemplates.Tests.csproj
  • /src/Security/test/AuthSamples.FunctionalTests/AuthSamples.FunctionalTests.csproj
  • /src/Servers/IIS/IIS/test/IISExpress.FunctionalTests/IISExpress.FunctionalTests.csproj
  • /src/Servers/Kestrel/test/InMemory.FunctionalTests/InMemory.FunctionalTests.csproj
  • /src/Servers/test/FunctionalTests/ServerComparison.FunctionalTests.csproj
  • /src/SignalR/clients/ts/FunctionalTests/FunctionalTests.csproj
  • /src/Tools/dotnet-watch/test/dotnet-watch.Tests.csproj

13 Individually Skipped tests

  • /src/Components/Blazor/Build/test/RuntimeDependenciesResolverTest.cs
  • (2) /src/DataProtection/Extensions/test/DataProtectionProviderTests.cs
  • (3) /src/Identity/ApiAuthorization.IdentityServer/test/Configuration/ConfigureSigningCredentialsTests.cs
  • (3) /src/Identity/ApiAuthorization.IdentityServer/test/Configuration/SigningKeysLoaderTests.cs
  • (2) /src/Identity/test/Identity.Test/IdentityUIScriptsTest.cs
  • /src/Servers/Kestrel/Kestrel/test/GeneratedCodeTests.cs
  • /src/Tools/FirstRunCertGenerator/test/CertificateManagerTests.cs

@HaoK HaoK requested a review from natemcmaster as a code owner January 15, 2019 23:40
@HaoK HaoK requested a review from Tratcher as a code owner January 16, 2019 07:35
@HaoK
Copy link
Member Author

HaoK commented Jan 17, 2019

Yay arcade fix unblocked running helix the real way again, I had to remove the mvc functional tests and the signalr client ts functional tests though since they failed with build errors as part of the helix gather, not sure what's special about those two, I skipped the from building helix payloads for now

@HaoK HaoK force-pushed the haok/master/helix branch from 89e7c8c to ab7ae5f Compare January 18, 2019 22:48
@BrennanConroy
Copy link
Member

I had to remove the mvc functional tests and the signalr client ts functional tests though since they failed with build errors as part of the helix gather

Need any help from SignalR folks to figure this out?

@HaoK
Copy link
Member Author

HaoK commented Jan 24, 2019

Sure @BrennanConroy that'd be great, you just need to turn BuildHelixPayload back on for the test project in this branch to investigate

@HaoK HaoK force-pushed the haok/master/helix branch from ff07e13 to 2e4316b Compare January 24, 2019 20:45
@Eilon Eilon added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 25, 2019
@HaoK
Copy link
Member Author

HaoK commented Jan 29, 2019

Helix tests are finally green including localdb tests as well. Functional tests and tests that rely on the sln are the biggest category of SkipOnHelix tests left

@HaoK
Copy link
Member Author

HaoK commented Jan 30, 2019

17 test projects are skipped completely on helix (14 functional + 2 mvc analyzers + dotnet watch) + 13 individually skipped tests (mostly looking for sln + cert tests on OSX). Rest of the tests look good now.

Copy link
Contributor

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

It looks like this is really close. I've left a few nit-picks and questions. The CI is failing normal builds with this change (obviously something to fix first), but other than that, I'd say this is ready to go.

@natemcmaster
Copy link
Contributor

natemcmaster commented Jan 31, 2019

Another question - any idea why we don't see the test failures under 'Tests' in AzDO? I thought the Helix SDK was going to report test failures to the UI.

image

@HaoK
Copy link
Member Author

HaoK commented Jan 31, 2019

@MattGal do you know what I need to do to get helix test failures to get reported to the AzDo UI that nate is mentioning?

@HaoK
Copy link
Member Author

HaoK commented Jan 31, 2019

@natemcmaster should we move the helix job to a separate checkbox similar to how pr-fastvalidation used to be instead of running it as part of normal ci to start?

@natemcmaster
Copy link
Contributor

should we move the helix job to a separate checkbox similar to how pr-fastvalidation used to be instead of running it as part of normal ci to start?

Yeah, that's probably a good idea while we stabilize Helix. I expect we'll find more flaky tests an account of how many times each test will run. If you move the YML changes to .azure/pipelines/helix-test.yml, I can setup a new AzDO definition.

@MattGal
Copy link
Member

MattGal commented Jan 31, 2019

@natemcmaster I believe to get this you should only need to set EnableAzurePipelinesReporter to true.

Relevent line in props file

@HaoK
Copy link
Member Author

HaoK commented Jan 31, 2019

@natemcmaster moved the helix job to its own yml file, and also turned on the azure reporting flag. Running time is also approx 20 min per queue, so running all 10 queues takes a while. Matt identified some obvious perf bottlenecks a few weeks ago, I'll see if I can do some of the low hanging ones to speed things up now that things are working

@natemcmaster
Copy link
Contributor

Thanks @MattGal!

@HaoK this looks awesome! Obviously there is a handful of work necessary to get all of our tests running in Helix, but I think we should get this merged soon. I'm about to merge #7137 which may have a few conflicts on ci.yml, but otherwise I think this should be good to merge today.

@HaoK
Copy link
Member Author

HaoK commented Jan 31, 2019

Yeah I'll rebase against master again now, I'm not sure why the normal ci would be failing after I do that since the helix stuff shouldn't be affecting it at all anymore

@HaoK HaoK force-pushed the haok/master/helix branch from 0dd670e to ec6b459 Compare January 31, 2019 21:41
@natemcmaster
Copy link
Contributor

My changes are in master now. Looks like the merge conflict is on FunctionalTests.csproj now.

@HaoK
Copy link
Member Author

HaoK commented Feb 6, 2019

Almost there, helix tests are all passing after skipping a few new failing tests, @natemcmaster can you add the system access token to the pipeline that helix is trying to access? Hopefully things will finally go green again once it has that.

2019-02-06T08:43:00.3652531Z D:\a\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19073.6\tools\azure-pipelines\AzurePipelines.props(27,5): 
error : Azure Pipelines SYSTEM_ACCESSTOKEN variable not found. Either the build is not running in Azure Pipelines, or the current step is not configured correctly. 
Please configure this step with the System.AccessToken variable, see: https://docs.microsoft.com/en-us/azure/devops/pipelines/process/variables?view=vsts&tabs=yaml,batch#systemaccesstoken [D:\a\1\s\test\helix.proj]

@pakrym
Copy link
Contributor

pakrym commented Feb 7, 2019

Green! 👍

@HaoK
Copy link
Member Author

HaoK commented Feb 7, 2019

Yeah, guess the azure error reporting thing is causing that new failure, I turned that flag off for now. retriggering the normal ci since that failed now :(

Copy link
Contributor

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

Awesome, let's do this. Let's open a bug to follow up on re-enabling AzDO reporting.

Last suggestions are nits about code organization, but I don't want to block the PR on this since this has been in progress for so long.

Nit 1 - I'd like to avoid adding a new top-level folder to this repo. We are using the eng/ folder for targets/tasks/scripts etc. eng/helix/ or eng/scripts/ seems like a better place for some of this code.

Nit 2 - put shared source code (SkipOnHelixAttribute.cs) in src/Shared/

@HaoK
Copy link
Member Author

HaoK commented Feb 7, 2019

Cool, I'll merge and start another PR with the tweaks, and it looks like there's a few flaky tests still too since the helix job still fails like half the time.

@HaoK HaoK merged commit c9499e1 into master Feb 7, 2019
@BrennanConroy
Copy link
Member

Now that this is merged and running on peoples PR's, an email to the team describing what it is, how it works, and how to see failures would be nice.

@BrennanConroy BrennanConroy deleted the haok/master/helix branch February 7, 2019 21:06
@HaoK
Copy link
Member Author

HaoK commented Feb 7, 2019

Good idea I'll send something out this afternoon

@MattGal
Copy link
Member

MattGal commented Feb 7, 2019

Awesome to see this merged you guys, you came a long way in a (relatively) short period of time on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants