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.

Sign up for GitHub

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

Jump to bottom

[tests] Failing test on CI - DistributedApplicationTests.ProxylessAndProxiedEndpointBothWorkOnSameResource #4599

Open
radical opened this issue Jun 19, 2024 · 3 comments · May be fixed by #7073
Open

[tests] Failing test on CI - DistributedApplicationTests.ProxylessAndProxiedEndpointBothWorkOnSameResource #4599

radical opened this issue Jun 19, 2024 · 3 comments · May be fixed by #7073
Assignees
@davidfowl
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication disabled-tests testing ☑️
Milestone
9.1

Comments

@radical
Copy link
Member

radical commented Jun 19, 2024

#4500 enables extra Aspire.Hosting.Tests to run on the build machine. Prior to this they were never run on CI.

Aspire.Hosting.Tests.DistributedApplicationTests.ProxylessAndProxiedEndpointBothWorkOnSameResource fails because the first client.GetStringAsync($"{httpEndPoint}urls" returned only the http endpoint. But the second invocation got both http, and https. Adding a sufficient delay before hitting the /urls endpoints makes the test pass though.

The tests have an existing class for the hook which I tried to use here - https://gist.github.com/radical/7ef257229178c36f6b72ffd09f7e0b79#file-test-cs-L22-L27

But running the test with CWLs added to print the results from /urls:

Got urls from http://localhost:1234/urls: ["http://localhost:1234"]
Got urls from https://localhost:57048/urls: ["http://localhost:1234","https://localhost:57051"]

The first call is missing the https endpoint. Did the AfterEndpointsAllocatedAsync get invoked too early? Or am I missing something else?

(related thread: #4500 (comment))
cc @ReubenBond @DamianEdwards @eerhardt
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jun 19, 2024
@radical radical changed the title DistributedApplicationTests.ProxylessAndProxiedEndpointBothWorkOnSameResource fails on CI [tests] Failing test on CI - DistributedApplicationTests.ProxylessAndProxiedEndpointBothWorkOnSameResource Jun 19, 2024
radical added a commit to radical/aspire that referenced this issue Jun 19, 2024
@radical
Disable failing test ProxylessAndProxiedEndpointBothWorkOnSameResource
fb788aa 
Issue: dotnet#4599
radical added a commit to radical/aspire that referenced this issue Jun 30, 2024
@radical
Squashed commit of the following:
2753f24 
commit 14b5e2f
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 20 14:55:40 2024 -0400

    Fix Aspire.Hosting.Testing.Tests.TestingFactoryTests.HttpClientGetTest by using httpclient with resilience

commit 251d59a
Merge: ac52e88 bae6c46
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 20 14:14:26 2024 -0400

    Merge remote-tracking branch 'origin/main' into hosting-testing-build

commit ac52e88
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 20 14:11:27 2024 -0400

    address review feedback from @ eerhardt

commit b1b0682
Author: Mitch Denny <[email protected]>
Date:   Wed Jun 19 22:36:45 2024 -0700

    Update container logs available condition to allow streaming build logs. (dotnet#4601)

commit f9e2eba
Author: David Negstad <[email protected]>
Date:   Wed Jun 19 17:17:55 2024 -0700

    Update additional DCP resource schema (dotnet#4600)

commit 023fd5c
Author: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Date:   Wed Jun 19 21:57:14 2024 +0000

    Update dependencies from https://github.com/microsoft/usvc-apiserver build 0.5.3 (dotnet#4598)

    [main] Update dependencies from microsoft/usvc-apiserver

commit fd2474e
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 20 14:12:39 2024 -0400

    Update tests/TestingAppHost1/TestingAppHost1.AppHost/TestingAppHost1.AppHost.csproj

    Co-authored-by: Eric Erhardt <[email protected]>

commit fe32fd3
Author: Ankit Jain <[email protected]>
Date:   Wed Jun 19 20:43:42 2024 -0400

    Use `b.AddStandardResilienceHandler`

commit c5f7f3e
Author: Ankit Jain <[email protected]>
Date:   Wed Jun 19 18:26:28 2024 -0400

    Add resilience handlers in the test itself

commit fb788aa
Author: Ankit Jain <[email protected]>
Date:   Wed Jun 19 17:16:23 2024 -0400

    Disable failing test `ProxylessAndProxiedEndpointBothWorkOnSameResource`

    Issue: dotnet#4599

commit fa3bce0
Merge: c0743ee 4e06e32
Author: Ankit Jain <[email protected]>
Date:   Wed Jun 19 17:12:14 2024 -0400

    Merge remote-tracking branch 'origin/main' into hosting-testing-build

commit c0743ee
Merge: 5e28b57 faf26a8
Author: Ankit Jain <[email protected]>
Date:   Wed Jun 19 16:12:32 2024 -0400

    Merge remote-tracking branch 'origin/main' into hosting-testing-build

commit 5e28b57
Merge: abad241 01f7399
Author: Ankit Jain <[email protected]>
Date:   Tue Jun 18 15:40:52 2024 -0400

    Merge remote-tracking branch 'origin/main' into hosting-testing-build

commit abad241
Merge: 4038329 c3d1e74
Author: Ankit Jain <[email protected]>
Date:   Tue Jun 18 13:11:13 2024 -0400

    Merge remote-tracking branch 'origin/main' into hosting-testing-build

commit 4038329
Merge: 4335fa0 c294706
Author: Ankit Jain <[email protected]>
Date:   Mon Jun 17 18:34:51 2024 -0400

    Merge remote-tracking branch 'origin/main' into hosting-testing-build

commit 4335fa0
Author: Ankit Jain <[email protected]>
Date:   Mon Jun 17 18:16:48 2024 -0400

    DistributedApplicationTests.cs: Bump timeout for docker based tests from 10secs for 1min

commit b32414e
Merge: 0742f8a 8e311f3
Author: Ankit Jain <[email protected]>
Date:   Mon Jun 17 15:05:08 2024 -0400

    Merge remote-tracking branch 'origin/main' into hosting-testing-build

commit 0742f8a
Merge: aa72d39 d447bda
Author: Ankit Jain <[email protected]>
Date:   Sat Jun 15 04:23:59 2024 -0400

    Merge remote-tracking branch 'origin/main' into hosting-testing-build

    # Conflicts:
    #	tests/Aspire.Components.Common.Tests/RequiresDockerAttribute.cs
    #	tests/Aspire.Hosting.Tests/Schema/SchemaTests.cs

commit aa72d39
Author: Ankit Jain <[email protected]>
Date:   Fri Jun 14 23:19:13 2024 -0400

    Remove SkipOnHelixTheory, and SkipOnHelixFact attributes

commit d2d7a18
Author: Ankit Jain <[email protected]>
Date:   Fri Jun 14 19:41:00 2024 -0400

    cleanup

commit 36654c0
Author: Ankit Jain <[email protected]>
Date:   Fri Jun 14 19:03:42 2024 -0400

    cleanup

commit 6fb3c82
Author: Ankit Jain <[email protected]>
Date:   Fri Jun 14 19:02:42 2024 -0400

    TestingAppHost1.AppHost: add default resilience handler

commit 322b8e1
Author: Ankit Jain <[email protected]>
Date:   Fri Jun 14 18:38:59 2024 -0400

    Add resilience handler to httpclient in Hosting.Testing tests

commit 61fd312
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 22:43:50 2024 -0400

    fix typo

commit 08a8299
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 22:43:31 2024 -0400

    Add RequiresTools attributes, and use for the node/npm tests

commit f4c4a77
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 21:54:45 2024 -0400

    Disable node/npm tests on CI as the binaries are not available

commit daccd46
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 21:26:08 2024 -0400

    cleanup

commit 6913816
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 21:24:49 2024 -0400

    [tests] Remove RequiresDocker from Hosting.Tests that don't need it

commit 106ed06
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 21:05:44 2024 -0400

    [tests] Split npm/nodeapp test

commit 502e36f
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 19:48:47 2024 -0400

    FIXME: add delay in ProxylessAndProxiedEndpointBothWorkOnSameResource to wait for all the endpoints to get allocated

commit a85b721
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 17:44:18 2024 -0400

    [tests] Fix VerifyNodeAppWorks by fixing the constructed path to nodeapp

commit cbc7897
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 17:40:24 2024 -0400

    [tests] FIXME: add delay to wait for the webapp to be ready

commit ca5d62f
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 17:37:25 2024 -0400

    [tests] Throw if application hasn't been initialized, useful for debugging issues

commit 78dc7a2
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 17:36:45 2024 -0400

    [tests] Install devcerts on linux/CI

commit fc2bf4f
Author: Ankit Jain <[email protected]>
Date:   Thu Jun 13 17:36:01 2024 -0400

    [tests] Use RequiresDocker instead of LocalOnlyFact/Theory attributes

    - this allows the tests to run on the build machine

commit 3d8949c
Author: Ankit Jain <[email protected]>
Date:   Wed Jun 12 21:30:41 2024 -0400

    [tests] Use .runsettings when running non-helix tests, so tests skipping can work

    To be specific, `category!=failing` is required.

commit 9c964a6
Author: Ankit Jain <[email protected]>
Date:   Wed Jun 12 21:28:43 2024 -0400

    [tests] RequiresDocker: Update condition to disallow docker based tests

    .. on windows/build-machine on CI.

# Conflicts:
#	eng/pipelines/templates/BuildAndTest.yml
#	tests/Aspire.Hosting.Testing.Tests/TestingBuilderTests.cs
#	tests/Aspire.Hosting.Testing.Tests/TestingFactoryCrashTests.cs
#	tests/Aspire.Hosting.Testing.Tests/TestingFactoryTests.cs
#	tests/Aspire.Hosting.Tests/SlimTestProgramTests.cs
@davidfowl
Copy link
Member

@radical Do we still need this issue?

@radical
Copy link
Member Author

radical commented Sep 16, 2024

@radical Do we still need this issue?

Yeah, looks like the test is still disabled.

@joperezr joperezr added the untriaged New issue has not been triaged label Oct 15, 2024
@davidfowl
Copy link
Member

OK I spent some time and figured this one out. The tests are just racy because kestrel binds to the input addresses and mutates the collection as its doing so. We can use health checks to wait for both to be bound (which is the cleaner solution) or look at ASPNETCORE_URLS

@davidfowl davidfowl self-assigned this Jan 14, 2025
@davidfowl davidfowl added this to the 9.1 milestone Jan 14, 2025
@davidfowl davidfowl removed the untriaged New issue has not been triaged label Jan 14, 2025
@davidfowl davidfowl linked a pull request Jan 16, 2025 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@davidfowl davidfowl

Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication disabled-tests testing ☑️
Projects
None yet
Milestone
9.1
Development

Successfully merging a pull request may close this issue.

Trying out gha to run tests dotnet/aspire
3 participants
@radical @davidfowl @joperezr