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

Test Integration: add basic and app restart actor timer tests #7534

Open
wants to merge 18 commits into
base: release-1.13
Choose a base branch
from

Conversation

JoshVanL
Copy link
Contributor

@JoshVanL JoshVanL commented Feb 13, 2024

Adds integration tests for basic timer functionality, as well as
ensuring a timer will not continue to be triggered after the application
has been marked as unhealthy.

Adds the following environment variables in order to speed up the actor
app being marked as unhealthy. This speeds the test to 4s from 20s
without the changes.

These environment variables will be removed as and when the actor health
check is merged into the app health check.

DAPR_EXPERIMENTAL_ACTOR_HEALTH_FAILURE_THRESHOLD
DAPR_EXPERIMENTAL_ACTOR_HEALTH_UNHEALTHY_INTERVAL
DAPR_EXPERIMENTAL_ACTOR_HEALTH_HEALTHY_INTERVAL
DAPR_EXPERIMENTAL_ACTOR_HEALTH_REQUEST_TIMEOUT

@JoshVanL JoshVanL requested review from a team as code owners February 13, 2024 13:02
@JoshVanL
Copy link
Contributor Author

/test-version-skew

@dapr-bot
Copy link
Collaborator

dapr-bot commented Feb 13, 2024

Dapr Version Skew integration test (dapr-sidecar-master - 1.12.5)

🔗 Link to Action run

Commit ref: 7775d6f

❌ Version Skew tests failed

Please check the logs for details on the error.

@dapr-bot
Copy link
Collaborator

dapr-bot commented Feb 13, 2024

Dapr Version Skew e2e test (dapr-sidecar-master - 1.12.5)

🔗 Link to Action run

Commit ref: 7775d6f

✅ Version Skew tests passed

@dapr-bot
Copy link
Collaborator

dapr-bot commented Feb 13, 2024

Dapr Version Skew integration test (control-plane-master - 1.12.5)

🔗 Link to Action run

Commit ref: 7775d6f

✅ Version Skew tests passed

@dapr-bot
Copy link
Collaborator

dapr-bot commented Feb 13, 2024

Dapr Version Skew e2e test (control-plane-master - 1.12.5)

🔗 Link to Action run

Commit ref: 7775d6f

✅ Version Skew tests passed

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 62.04%. Comparing base (121f5e4) to head (51789c4).
Report is 9 commits behind head on release-1.13.

Current head 51789c4 differs from pull request most recent head 0ead26c

Please upload reports for the commit 0ead26c to get more accurate results.

Files Patch % Lines
pkg/actors/actors.go 0.00% 32 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           release-1.13    #7534      +/-   ##
================================================
- Coverage         62.14%   62.04%   -0.10%     
================================================
  Files               245      245              
  Lines             22410    22440      +30     
================================================
- Hits              13926    13924       -2     
- Misses             7325     7355      +30     
- Partials           1159     1161       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JoshVanL JoshVanL marked this pull request as draft February 13, 2024 15:02
@JoshVanL JoshVanL force-pushed the test-integration-actors-timers branch from 7775d6f to c807d3a Compare February 13, 2024 18:23
Adds integration tests for basic timer functionality, as well as
ensuring a timer will not continue to be triggered after the application
has been marked as unhealthy.

Adds the following environment variables in order to speed up the actor
app being marked as unhealthy. This speeds the test to 4s from 20s
without the changes.

These environment variables will be removed as and when the actor health
check is merged into the app health check.

```
DAPR_EXPERIMENTAL_ACTOR_HEALTH_FAILURE_THRESHOLD
DAPR_EXPERIMENTAL_ACTOR_HEALTH_UNHEALTHY_INTERVAL
DAPR_EXPERIMENTAL_ACTOR_HEALTH_HEALTHY_INTERVAL
DAPR_EXPERIMENTAL_ACTOR_HEALTH_REQUEST_TIMEOUT
```

Signed-off-by: joshvanl <me@joshvanl.dev>
@JoshVanL JoshVanL force-pushed the test-integration-actors-timers branch from c807d3a to 065ba5a Compare February 13, 2024 18:25
@JoshVanL JoshVanL marked this pull request as ready for review February 13, 2024 18:25
@@ -63,6 +64,11 @@ const (

errStateStoreNotFound = "actors: state store does not exist or incorrectly configured"
errStateStoreNotConfigured = `actors: state store does not exist or incorrectly configured. Have you set the property '{"name": "actorStateStore", "value": "true"}' in your state store component file?`

envKeyHealthFailureThreshold = "DAPR_EXPERIMENTAL_ACTOR_HEALTH_FAILURE_THRESHOLD"
Copy link
Member

Choose a reason for hiding this comment

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

Have we agreed to the "DAPR_EXPERIMENTAL" as the official way to introduce env variables?

I don't have a problem with it. Just checking we had done the vetting on 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.

No we haven't, and I'd definitely like feedback for it. It seems like the right thing to do, but there may already exist a mechanism in Dapr I'm not aware of.

Ideally, we would merge the actor and app health checks into the same routine which would drop the need for this at all.

The main reason for this to exist is for integration tests not to take an unreasonable amount of time, though real users could see benefit from it in theory.

@JoshVanL JoshVanL added the autoupdate DaprBot will keep the Pull Request up to date with master branch label Mar 5, 2024
@JoshVanL JoshVanL added this to the v1.14 milestone Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoupdate DaprBot will keep the Pull Request up to date with master branch
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants