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

Isolated Entity Tests #2612

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

Conversation

sebastianburckhardt
Copy link
Collaborator

@sebastianburckhardt sebastianburckhardt commented Oct 2, 2023

Adds a separate function app for running entity tests.
The tests can be run (individually or all together) using http triggers.

Pull request checklist

PR checklist is tracked by feature branch.

* update durability provider class for new core-entities support.

* add configuration setting for max entity concurrency to DurableTaskOptions

* minor fixes.
* update DurableClient to take advantage of native entity queries if available

* fix minor errors.

* address PR feedback
* implement passthrough middleware for entities.

* propagate changes to protocol

* update/simplify protobuf format

* address PR feedback
* implement entity queries for grpc listener

* propagate changes to protocol

* update/simplify protobuf format
* durability provider must implement and pass-through IEntityOrchestrationService since it wraps the orchestration service

* simple mistake

* fix misunderstanding of initializer syntax (produced null, not empty list)

* fix missing failure details

* fix missing compile-time switch for trigger value type

* fix missing optional arguments

* fix  missing override
* add an entity example to the DotNetIsolated smoke test project.

* remove superfluous argument.

* address PR feedback
* Add worker side entity trigger and logic

* update comments

* Address PR comments
@sebastianburckhardt sebastianburckhardt added core-entities required for entity support in isolated P1 Priority 1 labels Oct 2, 2023
@sebastianburckhardt sebastianburckhardt changed the title Isolated Tests Isolated Entity Tests Oct 2, 2023
@sebastianburckhardt sebastianburckhardt marked this pull request as ready for review October 5, 2023 15:58
sebastianburckhardt and others added 7 commits October 5, 2023 15:24
* assign the necessary AzureStorageOrchestrationServiceSettings

* propagate changes to query name and metadata parameters

* add missing override for TaskOrchestrationEntityFeature
* add configuration for EnableEntitySupport

* rename includeStateless to includeTransient
Copy link
Contributor

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

Partial review. So far the testing harness sense. Just need to go over the tests themselves

{
public static async Task<string> RunAsync(TestContext context, string? filter = null, bool listOnly = false)
{
var sb = new StringBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would suggest to fully write out builder or stringBuilder here. I realize sb is a common shorthand here, but I think we usually try to avoid shorthands in this codebase.


internal static class TestRunner
{
public static async Task<string> RunAsync(TestContext context, string? filter = null, bool listOnly = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit surprised that we're writing our own test runner. I'm not strictly against it (plus this is relatively simple) but why why aren't we scaffolding this with our usual testing framework?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It has to run inside a function app. I had no appetite for spending a lot of time figuring out how to get a unit testing framework running inside a function app. Also, my past experience with those frameworks is not great. Takes a lot to get them to do what you want, even if what you want is rather simple. As demonstrated with this little bit of code here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I am not sure we should be adding our own test runner. What I recommend is using xunit test fixtures. You can encapsulate starting the function app via this fixture and then author tests which pull in and start the fixture (this starting the function app), and then dispatch HTTP to the function app to run a given scenario.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I understand what you are suggesting, we would additionally create a separate xunit test for each test and then have that test invoke the app's unit test via http call?

Would that be a separate project (given that the function app is a special kind of project)? Would there be any way to avoid having to redefine all tests in both projects?

},
"extensions": {
"durableTask": {
"entityMessageReorderWindowInMinutes": 0 // need this just for testing the CleanEntityStorage
Copy link
Contributor

Choose a reason for hiding this comment

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

does JSON parsing in .NET work with inlined comments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose it depend on what parser is used. Has worked for me without issues for host.json.

int state = await context.WaitForEntityStateAsync<int>(entityId);
Assert.Equal(1, state);

// entity still exists
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be: "entity now exists"? Instead of 'still exists'? It did not exist before, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah it is a bit misleading. Rewording.

@sebastianburckhardt
Copy link
Collaborator Author

We do not need this branch to be merged for the preview but it should be merged, and run in CI, before GA.

# Conflicts:
#	release_notes.md
#	src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableClient.cs
#	src/WebJobs.Extensions.DurableTask/WebJobs.Extensions.DurableTask.csproj
#	src/Worker.Extensions.DurableTask/AssemblyInfo.cs
#	src/Worker.Extensions.DurableTask/Worker.Extensions.DurableTask.csproj
#	test/SmokeTests/OOProcSmokeTests/DotNetIsolated/DotNetIsolated.csproj
@sebastianburckhardt sebastianburckhardt changed the base branch from feature/core-entities to dev February 20, 2024 19:23
@sebastianburckhardt
Copy link
Collaborator Author

I added two more tests that are not entity-related, but that can help us test and document the failure propagation for activities and suborchestrators.

…ner exceptions), and similarly for activity and orchestration error checking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-entities required for entity support in isolated P1 Priority 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants