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

Add a test for PurgeAllInstancesAsync #3021

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

andystaples
Copy link
Contributor

  • Adds an E2E test for PurgeAllInstancesAsync
  • Fix bug with PurgeAllInstancesAsync relating to passing a null start date

Issue describing the changes in this PR

resolves #2702

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
  • My changes do not require any extra work to be leveraged by OutOfProc SDKs
    • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
  • My changes do not change the version of the WebJobs.Extensions.DurableTask package
    • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
  • My changes do not add EventIds to our EventSource logs
    • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.
  • My changes should be added to v2.x branch.
    • Otherwise: This change applies exclusively to WebJobs.Extensions.DurableTask v3.x. It will be retained only in the dev and main branches and will not be merged into the v2.x branch.

andystaples and others added 2 commits January 30, 2025 13:29
- Fix bug with PurgeOrchestrationHistory relating to null start date
@andystaples andystaples requested review from cgillum, jviau, nytian and bachuv and removed request for cgillum January 30, 2025 20:36
Assert.Matches(responseRegex, actualMessage);
Assert.Equal(expectedStatusCode, response.StatusCode);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any other test cases that we can add? Maybe a test to cover the changes you made in ProtobufUtils.cs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test actually does cover that case - starttime is null in the function app code for this test, and before my change, it threw instead of returning.
Would love to add more tests, though. I think a good set to start with would be

  • A test with empty end date
  • A test with a start and end date
  • Purging from (now + 1 minute) to (now + 1 day) should delete 0 records
  • Starting an orchestration, then purging from >1 day ago should not purge the new orchestration's history
  • Starting an orchestration, waiting for it to complete, then purging all history should delete that orchestration's history

Does this seem reasonable? Other scenarios I should cover? @bachuv @cgillum

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds good to me. For the last three bullets, I remember Chris has a sample test for reference, I also referred to this when adding purge test at DTS

https://github.com/microsoft/durabletask-mssql/blob/ee4a99c31eff04d8f9595fcfeb5b5dd0a873a99f/test/DurableTask.SqlServer.AzureFunctions.Tests/CoreScenarios.cs#L311

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PurgeAllInstancesAsync throwing Grpc.Core.RpcException
3 participants