-
Notifications
You must be signed in to change notification settings - Fork 308
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
[tests] Workload tests #4168
[tests] Workload tests #4168
Conversation
df23466
to
abd154e
Compare
…y handles 'nothing running' case
- also show process run times which is useful in identifying hangs
c2f7e55
to
8f64d95
Compare
- also, deploy the chromium browser for helix - for this we use a `PlaywrightWrapper` project to invoke to run `Main` for `Playwright` and use it to provision the browser.
This can be used to prepare a shell environment for using the sdk+workload from `artifacts/bin/dotnet-latest`. `$ source /path-to-aspire-repo/dogfood.sh`
8f64d95
to
e7ffa9e
Compare
@@ -0,0 +1,28 @@ | |||
if [ -z "$ZSH_VERSION" ]; then |
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.
NIT: should probably have #!/usr/bin/env bash
at the top line of this script.
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.
Also, I don't see in other scripts in the repo us checking if you are using Bash or Zsh, is there a reason why we need that here?
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.
This is meant to be used like source dogfood.sh
, so that wouldn't be appropriate AFAIU.
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.
${BASH_SOURCE[0]}
is not supported in zsh
.
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 see, I think that having it be a .sh
will be misleading then if the intent is to do source dogfood.sh. Just curious, why not make it an actual bash script that can be executed? Here is precedent of a very similar use cas in the sdk repo: https://github.com/dotnet/sdk/blob/main/eng/dogfood.sh
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.
If the script is run as /foo/dogfood.sh
then the changes to the environment will be limited to that script. The documentation in sdk also suggests using source dogfood.sh
:
When using source ..
we are using the current shell to run the commands so the shebang should be not be needed, AFAIU.
…Directory.Build.props
…feedback from @ eerhardt
The template generators can handle odd characters in the project name, so this isn't needed anymore.
.. and instead use a pre-generated `runtimeconfig.json` to directly run `Microsoft.Playwright.dll`. Based on feedback from @ eerhardt .
|
||
<RunTestsOnHelix>true</RunTestsOnHelix> | ||
<TestUsingWorkloads>true</TestUsingWorkloads> | ||
<InstallWorkloadForTesting>true</InstallWorkloadForTesting> |
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.
<InstallWorkloadForTesting>true</InstallWorkloadForTesting> |
I don't think this is needed, since it defaults to TestUsingWorkloads
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 doesn't default to TestUsingWorkloads
.
The workload testing targets install two sdks - dotnet-latest
, and dotnet-none
. The former has the manifest+workload installed, but that latter has only the manifest installed. It is useful for running tests that want to check the case the user does not have the workload installed.
The TestUsingWorkloads
property essentially controls that. I'll open a PR in dotnet/runtime
to rename this property so its purpose is clearer. And possibly clean this up a bit.
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.
Tracking in #4222 .
</Exec> | ||
|
||
<PropertyGroup> | ||
<_Regex>^\s*(Aspire.Workload.Tests[^\($]+)</_Regex> |
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.
What happens if a new test gets added that doesn't follow this naming? Does the test just get skipped and never executed?
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.
Yeah :/ I'll open an issue to make this more robust.
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.
Tracking in #4222 .
{ | ||
public IEnumerable<KeyValuePair<string, string>> GetTraits(IAttributeInfo traitAttribute) | ||
{ | ||
if (!RequiresDockerTheoryAttribute.IsSupported) |
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.
(not necessarily for this PR) - it would be nice if we had a consolidated "RequiresDocker" story across all our tests. This feels pretty spread out - some in Aspire.Components.Common.Tests
, some here, some in Aspire.Hosting.Tests
.
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.
Tracking in #4222 .
Co-authored-by: Eric Erhardt <[email protected]>
Co-authored-by: Eric Erhardt <[email protected]>
This was used for inserting a source for the locally built nugets. But this was changed to instead have `<add key="built-local" value="%BUILT_NUGETS_PATH%" />` so the path comes from the environment variable `BUILT_NUGETS_PATH` now, and we can avoid needing to patch the nuget.config.
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.
Nice work here @radical! Let's get this in and we can iterate on it as we go.
This adds
Aspire.Workload.Tests
which are meant to exercise theaspire
workload. The individual tests create projects from templates just like a user would, and then run, and validate them.It uses the existing infrastructure from
Aspire.EndToEnd.Tests
to install sdk+workload for testing.Tests
Two kinds of tests are being added here:
aspire-starter
template project, and then some tests interact with that running app to validate itaspire-starter
with [xunit,nunit,mstest] optionsPlaywright
.aspire-starter
template to validate some parts of it.Fixes #47 .
Follow up work is tracked in #4222
Microsoft Reviewers: Open in CodeFlow