Refactoring and adding unit tests to deployment

[guimafelipe]

This Pull Request introduces a refactoring on the DeploymentManager.cpp class 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 Initialize method. More specifically, on the internal _Initialize method. _Initialize calls a internal method Deploy(..), which is a 3 lines method that calls InstallLicenses(...) and DeployPackages(...).

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::GetLicenseFiles and WindowsAppRuntime::Deployment::Deployer::InstallLicenses.

GetLicenseFiles

This 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.

InstallLicences

This function receives the list of licenses to install and uses the LicenseInstaller to 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 interface ILicenseInstaller that this function accepts as parameter. The DeployementManager::InstallLicenses implements 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::GetDeploymentPackageArguments and WindowsAppRuntime::Deployment::Deployer::DeployPackages.

GetDeploymentPackageArguments

This 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.

DeployPackages

This is the actual deployment. This function calls WindowsAppRuntime::Deployment::PackageRegistrar::AddOrRegisterPackageInBreakAwayProcess or WindowsAppRuntime::Deployment::PackageRegistrar::AddOrRegisterPackage depending on the arguments (in this case, if it is full trust or not). These two functions were moved to the PackageRegistrar namespace 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 files PackageUtilities.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.

[ssparach]

With the refactor of licensing-related logic (mostly in Deployer), why is the license path constructed here? Since the code is being refactored, it might be worth separating concerns by introducing distinct classes. For example, consider creating a dedicated LicenseManager or similar to encapsulate licensing responsibilities—to align with single-responsibility principles.

[guimafelipe]

I put the construction of the package path there because that is the entry point of all the deployment logic. In my point of view, this code does not need to be object oriented. It is a API of two functions (GetStatus and Initialize) living as static methods of the DeploymentManager class, which is basically a namespace at this point.

If you consider the whole API, both methods are entry points of a big procedure. We call them, they do something and then they finish the work. Most of the preparation for that work that it is needed to be done I think we can push out to the closest of the entry point as possible (for example, building the arguments), so the inner functions focus more on business logic. This way, it is easier to track the behavior of the inner functions and the we isolate the construction of the arguments they need to act closer to the entry point.

In this case, I think the single responsibility would be separating the creation of the arguments and data necessary to do the work (data instantiation) from the business logic. It makes the functions not need to reach to any external data to work or to keep any internal state that could generate side effects. We keep the functions the most testable as possible and remove from them responsibility to instantiate their own data.

edit: Another option is to have this method back to the Licensing namespace and the manager just calls the function that builds this license path. It passed through my mind but I felt it was unnecessary to add a method just to building a string (and it would not need any testing to it either). Usually I try to avoid using functions for simple tasks that will not be needed to isolate, test or reuse.

[ssparach]

Passing startupNotificationsLongRunningPlatformFunc as a function argument can make the interface harder to maintain. Could we instead include the header that defines this functionality and call it directly where needed? This would reduce indirection and simplify the design.

[guimafelipe]

The main problem for that is that would make it a hard dependency on generated files and really hard to test in a controlled environment. This also makes it really hard do compile as we need to drag the PushNotificationsLongRunningPlatform-Startup.h with it an all its dependencies to the compilation tree.

Passing a function pointer here is the easiest way to break this hard dependency. I like it because we can the function is an argument as any other and it is not an anonymous lambda. I think it gives flexibility to maintain it in the future as the hard dependency will not exist and the responsibility to pass the function bubbles up to the caller.

Another option was to implement link substitution for tests (using a different source file with the same name but created only for tests, and the linker uses it on the compilation). But I think that would be even more confusing. What do you think?

[ssparach]

Since the logic pertains to DeploymentResult, would it make sense to move this into DeploymentResult.cpp for better alignment and cohesion? This could help keep related functionality grouped together and improve maintainability.

[guimafelipe]

That could be done, but I don't think it is a behavior that general. Initially, it was not even a separated function. I separated it in case of we wanting to test it isolated. I don't think this function would make sense out of PackageRegistrar namespace. I would love to hear more of your opinions about it.

[ssparach]

Given that GetDeploymentPackageArgs is only invoked once and appears tightly coupled to its caller, does it need to be separated into its own method? Introducing this extra layer—along with multiple parameters like activityContext etc.—makes the flow less intuitive (especially for debugging)

[guimafelipe]

I did that to separate the logic (creating the arguments based on the arguments and the external constant variables) from the IO execution (the calling the deployment of the packages to install them on the machine).

This makes GetDeploymentPackageArgs testable independently of the DeployPackages. Its behavior depends only on its arguments and we track regressions to it independently from what happens on the actual deployment.

DeployPackages is a function that we don't need to test when separated from the arguments construction. Its only task is to check an if to call either one or another function. We would rather test these internal functions instead and track their behavior.

I opt to receive multiple parameters instead of accessing global variables (like the previous AcivityContext::Get() because it makes more obvious that nothing external from the function will affect its behavior and everything that matters are what the functions is receiving as its parameters. The dependencies were always there, it just makes them explicit.

[ssparach]

Given the scope of this refactor, I’d strongly recommend exercising the functionality manually end to end across all three scenarios as well—packaged, unpackaged, and AppContainer. It would also be valuable to add coverage for AppContainer scenarios and/or sample launch tests, even if that happens in a future PR.

[DrusTheAxe]

In the description "...depending on the arguments (in this case, if it is full trust or not)."

Isn't that based on if it's Centennial or not?

Package management had a restriction where the caller needed to be...

1. Packaged process + AppContainer + packageManagement capability
2. Unpackaged process + IL>=Medium
3. Packaged process + IL>=Medium + NOT Centennial
4. Packaged process + IL>=Medium + Centennial + caller's Publisher == target's Publisher

Thus only packages with Publisher=Microsoft Corporation could do packagement management operations on WinAppSDK packages :-( and thus, Breakaway to avoid that constraint.

That restriction was lifted in 24H2 (Ge), but the Breakaway workaround is still very much required for downlevel.

RECOMMEND: Rephrase the description (either in painful detail, or more tersely and point to the source code for the full explanation). The latter's preferable IMO as it makes the code self-documenting - better for the long haul

[guimafelipe]

In the description "...depending on the arguments (in this case, if it is full trust or not)."

Isn't that based on if it's Centennial or not?

Package management had a restriction where the caller needed to be...

1. Packaged process + AppContainer + packageManagement capability
2. Unpackaged process + IL>=Medium
3. Packaged process + IL>=Medium + NOT Centennial
4. Packaged process + IL>=Medium + Centennial + caller's Publisher == target's Publisher

Thus only packages with Publisher=Microsoft Corporation could do packagement management operations on WinAppSDK packages :-( and thus, Breakaway to avoid that constraint.

That restriction was lifted in 24H2 (Ge), but the Breakaway workaround is still very much required for downlevel.

RECOMMEND: Rephrase the description (either in painful detail, or more tersely and point to the source code for the full explanation). The latter's preferable IMO as it makes the code self-documenting - better for the long haul

This was a bit confusing to me while comparing the documentation to what was actually in the code. For example: the final lines of the spec description says that "Alternatively, unpackaged apps that are full trust or MSIX packaged apps that have the packageManagement restricted capability can use the Deployment API to get these packages installed.".

But, on the actual code that is live in main, we have this:

// If the current application has runFullTrust capability, then Deploy the target package in a Breakaway process.
// Otherwise, call PackageManager API to deploy the target package.
// The Singleton package will always set true for forceDeployment and the running process will be terminated to update the package.
if (initializeActivity.GetIsFullTrustPackage())
{


    RETURN_IF_FAILED(AddOrRegisterPackageInBreakAwayProcess(packagePath, useExistingPackageIfHigherVersion, forceDeployment || isSingleton));
}
else
{
    RETURN_IF_FAILED(AddOrRegisterPackage(packagePath, useExistingPackageIfHigherVersion, forceDeployment || isSingleton));
}

We are choosing based on if it is "Full trust" or not. And we set the full trust here:

auto& initializeActivityContext{ ::WindowsAppRuntime::Deployment::Activity::Context::Get() };
const bool isPackagedProcess{ AppModel::Identity::IsPackagedProcess() };
const int integrityLevel = Security::IntegrityLevel::GetIntegrityLevel();
if (isPackagedProcess && integrityLevel >= SECURITY_MANDATORY_MEDIUM_RID)
{
    initializeActivityContext.SetIsFullTrustPackage();
}


::WindowsAppRuntime::Deployment::Activity::Context::Get().SetIsFullTrustPackage();

This does match what I understood by the documentation, there is no mention of the capabilities that are told as needed. And notice that the line right after the if clause sets full trust for everything (which I believe it is a bug and I have a Pull Request addressing it).

So, what I put in the description was following what is in the code behavior currently. Do you think this need to be changed?

[DrusTheAxe]

If the current application has runFullTrust capability

This is incorrect. RuntimeBehavior="packagedClassicApp" (aka Centennial) needs Breakaway, RuntimeBehavior="win32App" (aka Win32alacarte) doesn't.

[guimafelipe]

Given the scope of this refactor, I’d strongly recommend exercising the functionality manually end to end across all three scenarios as well—packaged, unpackaged, and AppContainer. It would also be valuable to add coverage for AppContainer scenarios and/or sample launch tests, even if that happens in a future PR.

I completely agree that all the scenarios need to be tested and I think we should have all of it automated instead of testing it manually. I think it is a worth effort to ensure it on the goal of reducing all the current errors and crashes from the deployment area.