-
Notifications
You must be signed in to change notification settings - Fork 447
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
Enable scenario test execution in VMR build #19222
Conversation
Can we please wait until #19090 is merged? That's pretty close and it will cause a conflict. |
I was playing around with the dev UX a bit and was surprised at the overhead prior to when it appears to actually build the test projects in a VMR that was previously built. Looking through the output it looks like it is creating the package archives every time I run ./build.sh --test -sb. It doesn't seem right that this logic is running in this scenario.
|
This seems outside the context of testing. Does this happen when you don't use the |
It seems there are two issues here. One, why is build even happening - is there not a way to avoid rebuilding? This feels related to arcade not being used. w/arcade --build is separate from --test so you have the ability to do both together or independently. Two, the symbols should not be repackaged when the build outputs haven't changed. I agree the later is outside the scope here. |
That's what the |
<Project Sdk="Microsoft.Build.NoTargets"> | ||
<PropertyGroup> | ||
<TargetFramework>$(NetCurrent)</TargetFramework> | ||
<IsTestProject>true</IsTestProject> |
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.
Just for my own understanding, why did you need to add this property?
cross-build of using Mariner to build for Alpine (linux vs linux-musl). --> | ||
<_RunScenarioTests Condition="'$(BuildOS)' != 'windows' and '$(__PortableTargetOS.ToLower())' != '$(TargetOS.ToLower())'">false</_RunScenarioTests> | ||
|
||
<!-- The scenario tests are not supported when unofficial build versioning is 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.
Please file a tracking issue for that and link to it 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.
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.
Looks great now. I went ahead and deleted src/SourceBuild/content/test/scenario-tests.proj
which was a leftover to avoid others reviewing the file.
IMO this is ready to be merged after you addressed my remaining minor feedback and filed an issue to track running scenario-tests in non-official builds.
Thanks for your continued effort on this.
A new error is showing up from the Aspire test. This isn't specific to my changes. I can repro it using the latest built SDK from the unified build pipeline. This seems specific to the unified build though. I can't repro it using an SDK built from the official installer pipeline. This error also doesn't occur in the source build pipeline from this PR.
@Forgind - It looks like you work in the workload space. Can you help point where to look in diagnosing this error? |
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Now the latest build shows unified build passing but source build is failing with a similar error, but referring to a different package:
That would seem to imply the issue is intermittent. |
That's interesting. The workload resolver supports there being multiple workload set files. If there are, it reads them all in and composes them, erroring if there are any duplicate entries because we clearly can't have two different versions of a single workload at the same time. It appears that in this case, we're loading the same workload set twice, which means that there are definitely going to be duplicates—in fact, the whole file should be duplicates. So I see two bugs here: first, we should accept duplicates as long as they're the exact same version (basically drop the error in this case). Second, we shouldn't be loading the same manifest twice. The first is an easy change. The second will take more investigation, as I don't know why we're trying to load the same manifest twice...glancing very quickly through the code, I don't see a way that's possible, but I didn't look exhaustively. Perhaps this has something to do with the overlay resolver? Perhaps something external is using reflection to call LoadManifestsFromProvider without updating _initializedManifests? |
Fixes dotnet/source-build#3819
Fixes dotnet/source-build#4278
This enables running the scenario tests via the build scripts.
There will be more follow up work to better integrate the SB smoke tests with the scenario tests as there is some duplication going on there. But for source build scenarios, both test suites are executed.
Both the SB and UB jobs are configured to enable the running of these tests.
It looks like the unified-build has some issues with F# in the OSX jobs that will need to be looked at later.
Test runs (internal links):