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

Update Integration Tests #1540

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Jan 4, 2024

  1. Add retry / resilience logic to the services that have slow starting containers (MySQL, Oracle, and SqlServer). Each of these containers can take more than 30 seconds to start on my machine.
  2. Remove unnecessary containers from the IntegrationServiceA project. Adding double the containers make the startup take even longer. So just use AddXYX resources and remove AddXYZContainer services.
  3. A couple other cosmetic changes - Use "rabbitmq" vs. "rabbit" - Log the responseContent in the SqlServer test

This won't be the last refactoring to these tests, but I think it is a step in the right direction.

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jan 4, 2024
@mitchdenny
Copy link
Member

How do we ensure that the AddXyzContainer resources don't bit rot

@eerhardt
Copy link
Member Author

eerhardt commented Jan 4, 2024

How do we ensure that the AddXyzContainer resources don't bit rot

Other tests.

These tests weren't ensuring those resources don't bit rot - because they weren't being used.

@eerhardt
Copy link
Member Author

eerhardt commented Jan 4, 2024

Maybe one thing we could do next is add another bool to CreateTestProgram that would switch between AddXYZ and AddXYZContainer. Then the same tests could run against both. But at any given time, only 1 set of resources would be created - either AddXYZ or AddXYZContainer.

Thoughts?

@mitchdenny
Copy link
Member

Other tests.

I looked through some of the resource specific functional tests and they seem to rely on the TestProgram as well to spin up the containers. With this change none of them will be testing AddXYZContainer(...) variants.

What the top level test relied upon (incorrectly in some instances) was the fact that we had healthchecks registered, so by hitting the healthcheck endpoint we could verify that the integrations were working (some components no-op healthchecks without additional config).

@davidfowl
Copy link
Member

We should get this merged @mitchdenny. What do you think is missing or needs to happen?

@mitchdenny
Copy link
Member

We need to either break up the integration test so they are smaller and focused on smaller groups of components or we need to read the AddXYZContainer extensions.

If we don't I don't think we've got an integration test that executes that code path.

I think with a larger timeout and improved retry logic it should be okay. And if there is instability starting larger groups of services we want to know about it.

In time I suspect that we'll end up with multiple integration test projects.

@eerhardt
Copy link
Member Author

@mitchdenny - can I get your eyes on this? The tests each pass individually on my machine.

My next step will be to split the Aspire.Hosting.Tests apart. We will leave the "unit tests" for Aspire.Hosting in Aspire.Hosting.Tests. And these integration tests will move to a new "EndToEnd" functional tests by themselves.

1. Add retry / resilience logic to the services that have slow starting containers (MySQL, Oracle, and SqlServer). Each of these containers can take more than 30 seconds to start on my machine.
2. Remove unnecessary containers from the IntegrationServiceA project. Adding double the containers make the startup take even longer. So just use AddXYX resources and remove AddXYZContainer services.
3. A couple other cosmetic changes
    - Use "rabbitmq" vs. "rabbit"
    - Log the responseContent in the SqlServer test
…o start.

Remove retries in Cosmos test. We don't need to retry at the test side, retries should be done inside the integration app.
Use non-Container overloads
Only retry on connect.
@eerhardt eerhardt enabled auto-merge (squash) February 1, 2024 17:52
@eerhardt eerhardt merged commit 9b6a1a6 into dotnet:main Feb 1, 2024
8 checks passed
radical pushed a commit to radical/aspire that referenced this pull request Feb 2, 2024
* Update Integration Tests

1. Add retry / resilience logic to the services that have slow starting containers (MySQL, Oracle, and SqlServer). Each of these containers can take more than 30 seconds to start on my machine.
2. Remove unnecessary containers from the IntegrationServiceA project. Adding double the containers make the startup take even longer. So just use AddXYX resources and remove AddXYZContainer services.
3. A couple other cosmetic changes
    - Use "rabbitmq" vs. "rabbit"
    - Log the responseContent in the SqlServer test

* Add more time for Cosmos, since the emulator can take over a minute to start.

Remove retries in Cosmos test. We don't need to retry at the test side, retries should be done inside the integration app.

* PR feedback

Use non-Container overloads
Only retry on connect.
eerhardt added a commit to eerhardt/aspire that referenced this pull request Feb 2, 2024
These connections were renamed in dotnet#1540, but were missed here.
radical pushed a commit that referenced this pull request Feb 2, 2024
These connections were renamed in #1540, but were missed here.
radical pushed a commit to radical/aspire that referenced this pull request Feb 6, 2024
* Update Integration Tests

1. Add retry / resilience logic to the services that have slow starting containers (MySQL, Oracle, and SqlServer). Each of these containers can take more than 30 seconds to start on my machine.
2. Remove unnecessary containers from the IntegrationServiceA project. Adding double the containers make the startup take even longer. So just use AddXYX resources and remove AddXYZContainer services.
3. A couple other cosmetic changes
    - Use "rabbitmq" vs. "rabbit"
    - Log the responseContent in the SqlServer test

* Add more time for Cosmos, since the emulator can take over a minute to start.

Remove retries in Cosmos test. We don't need to retry at the test side, retries should be done inside the integration app.

* PR feedback

Use non-Container overloads
Only retry on connect.
radical pushed a commit to radical/aspire that referenced this pull request Feb 6, 2024
These connections were renamed in dotnet#1540, but were missed here.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants