-
Notifications
You must be signed in to change notification settings - Fork 10.3k
[Helix] Shared framework support + Templates tests #19177
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
Conversation
@HaoK do you still need me to look at why IIS tests are failing here now? |
Yeah, I'm close to having an initial set of the templates tests working ( the easy ones), but we'll need to figure out why the IIS tests are failing before this PR can be merged. The templates tests are failing on the regular runs too right now, so something in the build might be messed up |
47cef0c
to
c3aeed1
Compare
Getting closer, looks like a few template tests are failing on mac and win7 and not everywhere else, mac failure looks to be cert related? @Tratcher can you help route these?
Grpc failure on Win7 looks like its just not supported? Does that template test need to be conditioned on something?
|
@HaoK gRPC isn't supported on MAC or Win7. They already have built in skips: aspnetcore/src/ProjectTemplates/test/GrpcTemplateTest.cs Lines 42 to 55 in b296f0c
The Win7 error matches the expected failure in the test, not sure why it didn't handle it. The MacOS failure is interesting, it's missing the dev cert. It's not clear from the test code how it's supposed to provision the dev cert. |
Guess those RuntimeInfo checks aren't working well on helix, I'll just slap on SkipOnHelix queues for the specific queues for the grpc tests |
I don't know if it is related but we've recently changed the gRPC template's launchSettings.json to include an HTTP endpoint to help macOS devs. |
Hmm, I wonder why running the template test on macOS passed after that update. I thought it only gave the error "HTTP/2 over TLS is not supported on macOS due to missing ALPN support." if HTTPS was the only endpoint. |
fda3c00
to
222cf33
Compare
Ok looks like we have a stable initial set of template tests working, there are still a bunch disabled, so I kept the azdo job running the template tests as well until they are full switched over. This is an initial checkpoint that I'd like to get merged though, since it brings over the shared framework, and runs most of the simple template tests. The ones that use identity and razor compilation are still failing due to restore issues (EF/compilation dll). I'll file issues to track re enabling the rest of those tests. But might as well start getting this reviewed |
? GetAssemblyMetadata("ArtifactsLogDir") | ||
: Path.Combine(Environment.GetEnvironmentVariable("HELIX_DIR"), "logs"); | ||
|
||
// FIGURE OUT EF PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File an issue tracking this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#19327 is the meta issue tracking all of the various skipped/remaining work
.azure/pipelines/ci.yml
Outdated
@@ -655,15 +657,19 @@ stages: | |||
|
|||
- template: jobs/default-build.yml | |||
parameters: | |||
condition: ne(variables['Build.Reason'], 'PullRequest') | |||
# condition: ne(variables['Build.Reason'], 'PullRequest') UNCOMMENT after testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this bit ready to review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the only way to test the daily/arm queues is by commenting out the conditions, I'll undo them once everything is ready to go
60c0a06
to
70ad733
Compare
@dougbu are you good with merging this once everything is green, I will undo the daily and arm conditions as well before merging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this looks fine modulo the two things you said you'd undo before merging
Cool merged this initial checkpoint, I'll open a new PR |
Rebased version of #16828