-
Notifications
You must be signed in to change notification settings - Fork 395
Refactoring and adding unit tests to deployment #5853
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
base: main
Are you sure you want to change the base?
Conversation
address PR comments
…lipeda/deploymenttests2
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.
Why test/DeploymentUnitTests/ and not test/Deployment/...whatever... (like, the current test/Deployment/API)?
test/foo should match dev/foo. Under that is fair game as the feature desires
Moved it to the correct directory. |
This Pull Request introduces a refactoring on the
DeploymentManager.cppclass implementation. The goal is to facilitate bug tracking and the implementation of more localized tests, making it easier to implement new changes, tracking possible regressions in inner behaviors and also on isolating the causes of existing failures.New modules
For this change if focused on the
Initializemethod. More specifically, on the internal_Initializemethod._Initializecalls a internal methodDeploy(..), which is a 3 lines method that callsInstallLicenses(...)andDeployPackages(...).These two methods were the main target of the refactoring. Let's go into each of them in details next.
Install licenses
This method is responsible for installing the licenses for the packages (Main and Singleton (and DDLM)) before installing them.
I separated it and extracted to a new namespace as the functions
WindowsAppRuntime::Deployment::Deployer::GetLicenseFilesandWindowsAppRuntime::Deployment::Deployer::InstallLicenses.GetLicenseFilesThis function's responsibility is to just iterate in the filesystem and return an array with the license files to install. Can be independently tested from the actual installation of the licenses.
InstallLicencesThis function receives the list of licenses to install and uses the
LicenseInstallerto execute the installation. The license installer is a stubbed file in the repository and we don't have access to its implementation. For this, I added an interfaceILicenseInstallerthat this function accepts as parameter. TheDeployementManager::InstallLicensesimplements a simple proxy/wrapper class around the real object and passes to the function. This gives us full control for mocking the installer on our tests and also a layer of separation for this namespace to not have strong dependency on the installer.Deploy Packages
This method is responsible for doing the actual deployment after the licenses were installed. I also divided this method in 2 functions with separated responsibilities:
WindowsAppRuntime::Deployment::Deployer::GetDeploymentPackageArgumentsandWindowsAppRuntime::Deployment::Deployer::DeployPackages.GetDeploymentPackageArgumentsThis function is responsible for building the vector with the package arguments that we will use later to execute the deployments. With this refactor, this becomes a pure function with very predictable behavior and no side effects.
DeployPackagesThis is the actual deployment. This function callsWindowsAppRuntime::Deployment::PackageRegistrar::AddOrRegisterPackageInBreakAwayProcessorWindowsAppRuntime::Deployment::PackageRegistrar::AddOrRegisterPackagedepending on the arguments (in this case, if it is full trust or not). These two functions were moved to thePackageRegistrarnamespace as they are similar and very specific on what they intent to do.Package utilities
The deployment manager's methods related to package querying were moved to a new namespace
WindowsAppRuntime::Deployment::Package, on the filesPackageUtilities.h|cpp. This was done in case we want to mock this behavior in tests and also for organization purposes.Unit tests
With this refactoring, I created the new test project
DeploymentUnitTests. I wrote some initial tests for both deployer and package registrar namespaces. With more test scenarios also being easily implemented if needed as now the modules are more independent and the major dependencies are broken.A microsoft employee must use /azp run to validate using the pipelines below.
WARNING:
Comments made by azure-pipelines bot maybe inaccurate.
Please see pipeline link to verify that the build is being ran.
For status checks on the main branch, please use TransportPackage-Foundation-PR
(https://microsoft.visualstudio.com/ProjectReunion/_build?definitionId=81063&_a=summary)
and run the build against your PR branch with the default parameters.