-
Notifications
You must be signed in to change notification settings - Fork 272
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 integration test framework for isolated #3000
base: dev
Are you sure you want to change the base?
Add integration test framework for isolated #3000
Conversation
- One app with basic "Hello Cities" scenario
2e4c00e
to
9a68303
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest a slightly revised file structure for this, basically putting all end-to-end related code under a test/e2e
directory.
/test/e2e/Apps/DotNetIsolated/App.csproj
/test/e2e/Apps/DotNetIsolated/...
/test/e2e/Tests/E2ETests.csproj
/test/e2e/Tests/...
In the future, we can add other language-specific apps under the /test/e2e/Apps/ directory too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR!
<FrameworkReference Include="Microsoft.AspNetCore.App" /> | ||
<PackageReference Include="Microsoft.Azure.Functions.Worker" Version="1.21.0" /> | ||
<PackageReference Include="Microsoft.Azure.WebJobs.Extensions.DurableTask" Version="3.1.2" /> | ||
<PackageReference Include="Microsoft.Azure.Functions.Worker.Extensions.DurableTask" Version="1.3.2" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add a project reference to the Worker Extensions package like this example instead of referencing a specific version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this would solve several issues with the current solution, including:
- Dynamic modifying of app.csproj with the build script will lead to unnecessary noise in commits and/or work excluding these files from unrelated PRs
- Would remove the requirement of manually bumping extension versions to run test locally
- Would simplify the build script
Need to explore this further. The extension build process in isolated has some gotchas but it may be possible to work around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would definitely be better to do a project reference as I mentioned in a separate comment, but we can do that as part of a follow up PR if it's not a simple fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we resolve the warnings that are showing up in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be done - let me know if you see specific new ones or ones I missed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a simple README for this project explaining the scripts that we have and how the fixtures and helper files are used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I focused more on the build scripts and practical usage steps, happy to add some info on fixtures and helpers if needed but for the most part, the goal is to abstract them to where adding or updating tests will not require interacting with those files at all
- Restructure e2e test folder structure - Add e2e test readme/build instructions - Move installed temp files to $env:TEMP or equivalent - Do as many checks as possible before creating Process() object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great. I've added some comments, but I don't think we necessarily need to block this PR on any of them.
var response = await GetResponseMessage(request); | ||
|
||
Console.WriteLine( | ||
$"InvokeHttpTrigger: {functionName}{queryString} : {response.StatusCode} : {response.ReasonPhrase}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will console logs be captured in the test output? Normally we write logs in Xunit to ITestOutputHelper
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting this whole method as it is never used and - IMO - we should rely on the Task method and validate responses in tests or methods defined in test classes
string actualMessage = await response.Content.ReadAsStringAsync(); | ||
Console.WriteLine( | ||
$"InvokeHttpTrigger: expectedMessage : {expectedMessage}, actualMessage : {actualMessage}"); | ||
return actualMessage.Contains(expectedMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could/should we use Assert.Contains(...)
here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting this whole method as it is never used and - IMO - we should rely on the Task method and validate responses in tests or methods defined in test classes
<ItemGroup> | ||
<FrameworkReference Include="Microsoft.AspNetCore.App" /> | ||
<PackageReference Include="Microsoft.Azure.Functions.Worker" Version="1.21.0" /> | ||
<PackageReference Include="Microsoft.Azure.WebJobs.Extensions.DurableTask" Version="3.0.2" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason to include the WebJobs package reference here? Normally .NET Isolated apps should never reference WebJobs packages directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will address along with the work to update to package references
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - now the test app has a reference to the worker extension project only. The webjobs extension is still packed and put in a local nuget packages folder alongside the app, so that the WorkerExtensions.csproj can get the version it needs
Direct commit applicable PR suggestions Co-authored-by: Chris Gillum <[email protected]>
- file-scoped namespaces - Remove unneeded .gitignore lines - using statements for HTTP objects where applicable - small readme improvements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! As a follow up to this PR, let's create another PR to run these tests as part of the CI.
} | ||
|
||
# Write-Host "Updating WebJobs.Extensions.DurableTask version to $webJobsExtensionVersion" | ||
# dotnet add app.csproj package Microsoft.Azure.WebJobs.Extensions.DurableTask --version $webJobsExtensionVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these commented-out lines be removed?
Pull request checklist
pending_docs.md
release_notes.md
/src/Worker.Extensions.DurableTask/AssemblyInfo.cs