-
Notifications
You must be signed in to change notification settings - Fork 446
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 scenario tests project #17636
Add scenario tests project #17636
Conversation
@@ -21,6 +21,8 @@ | |||
<RepositoryReference Include="source-build-reference-packages" /> | |||
<RepositoryReference Include="arcade" /> | |||
|
|||
<RepositoryReference Include="scenario-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.
This should be listed after source-build-externals
, right? Since a dependency was declared on it scenario-tests.proj.
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 should not be considered a toolset project. Toolset projects are the special bootstrapping tool projects used by everything else. I think this should be at the very end of the list - you want it to use all built packages and nothing from n-1. Perhaps this can go in it's own section
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 lists its dependencies (which are minimal) in the RepositoryReference section of the repo project. This is really what each project should do in order to enable parallelism, and this project would only list the roots of the dependency tree (dotnet/source-build#3608), which would end up as installer and scenario-tests, IIRC.
@@ -21,6 +21,8 @@ | |||
<RepositoryReference Include="source-build-reference-packages" /> | |||
<RepositoryReference Include="arcade" /> | |||
|
|||
<RepositoryReference Include="scenario-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.
This should not be considered a toolset project. Toolset projects are the special bootstrapping tool projects used by everything else. I think this should be at the very end of the list - you want it to use all built packages and nothing from n-1. Perhaps this can go in it's own section
<ItemGroup> | ||
<RepositoryReference Include="source-build-externals" /> | ||
<RepositoryReference Include="source-build-reference-packages" /> | ||
<RepositoryReference Include="arcade" /> |
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 in theory should depend on everything - perhaps just declaring a dependency on installer is sufficient.
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 actually need to depend on everything. It has no dependencies on the installer or runtime or anything like that for building. Only arcade, SBE and SBRP.
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.
The project executions would depend on the installed SDK, but not to build the harness and the xunit code.
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.
Right I get that but from a build perspective it could depend on all repos. You want it to compile against the latest libraries for example. Declaring a dependency on installer feels maintainable versus declaring all repo dependencies.
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.
That's not how the tests work though. The test projects are not supposed to build at VMR build time. They build using on an installed product. So for instance, with the template tests, new projects are created using the installed SDK, then built at test execution time.
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.
The only dependencies that the repo build has are SBE and SBRP, and arcade. There are no other package refs that would be used at scenario-tests's repo build time.
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.
Yes, of course - I wasn't thinking about this correctly.
@@ -49,6 +49,10 @@ | |||
|
|||
<!-- Package source-build artifacts --> | |||
<RepositoryReference Include="package-source-build" /> | |||
|
|||
<!-- Testing. --> | |||
<RepositoryReference Include="scenario-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.
Does this have the potential to cause issues when we introduce parallel builds? Specifically will a dependency need to be declared on package-source-build to ensure this is built after the package-source-build project? If this could be built in parallel or before the package project then the test's nupkg could end up in the source-built artifacts tarball. Worst case is non-deterministic behavior. I don't think we want these tests in the artifacts tarball.
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.
When introducing parallel builds, we only need to have dotnet.proj depend on the 'top-level' projects. I think it's that's package-source-build? Maybe scenario-tests too.. They all then depend on their dependencies. We enable parallel builds of dependencies and scenario tests could get built very early.
Part of the parallel build work will require that we create separate inputs for each component so that we don't end up with race conditions or unintentional dependencies. That would mean that package-source-build does not depend on scenario tests, and would not see the outputs of scenario tests as its inputs.
This is described in the SB issue.
Add the scenario tests project to the VMR build. Doesn't yet add test execution.